JFFS2 ACL problem.

[KaiGai Kohei]

Hi,

The attached patch separate jffs2_init_acl() into two parts.

The one is jffs2_init_acl_pre() called from jffs2_new_inode().
It compute ACL oriented inode->i_mode bits, and allocate in-memory ACL
objects associated with the new inode just before when inode meta
infomation is written to the medium.

The other is jffs2_init_acl_post() called from jffs2_symlink(),
jffs2_mkdir(), jffs2_mknod() and jffs2_do_create().
It actually writes in-memory ACL objects into the medium next to
the success of writing meta-information.

In the current implementation, we have to write a same inode meta
infomation twice when inode->i_mode is updated by the default ACL.
However, we can avoid the behavior by putting an updated i_mode
before it is written at first, as jffs2_init_acl_pre() doing.

Please review this patch.

Thanks,

KaiGai Kohei wrote:
> David Woodhouse wrote:
>> On Mon, 2007-08-20 at 20:17 +0900, KaiGai Kohei wrote:
>>> I also think that to split jffs2_init_acl() is a good idea to avoid
>>> the problem.
>>>
>>> In addition, this approach will enable to reduce the times to write
>>> a same inode, when inode->i_mode is changed on inode creation.
>> OK, that's what I've done. Does it look OK to you?
>>
>> http://git.infradead.org/?p=mtd-2.6.git;a=commitdiff;h=9ed437c50d89eabae763dd422579f73fdebf288d
> 
> I think it works fine. However, I have a comment to improve it.
> 
> The following part come from jffs2_init_acl() in the older version, isn't it?
> I think that most of the initialization of ACL should be done in this former
> section, except for actual writing on the medium.
> --------------------------------------------------------------
> @@ -431,7 +435,23 @@ struct inode *jffs2_new_inode (struct inode *dir_i, int mode, struct jffs2_raw_i
>         } else {
>                 ri->gid = cpu_to_je16(current->fsgid);
>         }
> -       ri->mode =  cpu_to_jemode(mode);
> +
> +       /* POSIX ACLs have to be processed now, at least partly.
> +          The umask is only applied if there's no default ACL */
> +       if (!S_ISLNK(mode)) {
> +               *acl = jffs2_get_acl(dir_i, ACL_TYPE_DEFAULT);
> +               if (IS_ERR(*acl)) {
> +                       make_bad_inode(inode);
> +                       iput(inode);
> +                       inode = (void *)*acl;
> +                       *acl = NULL;
> +                       return inode;
> +               }
> +               if (!(*acl))
> +                       mode &= ~current->fs->umask;
> +       } else {
> +               *acl = NULL;
> +       }
>         ret = jffs2_do_new_inode (c, f, mode, ri);
>         if (ret) {
>                 make_bad_inode(inode);
> --------------------------------------------------------------
> 
> In my opinion, the default ACL came from the parent directory should be
> set on f->i_acl_access or f->i_acl_default soon, and posix_acl_equiv_mode()
> is called to compute appropriate mode bits before the new inode is written
> at first.
> When mode bits are updated via posix_acl_equiv_mode(), the new inode is
> written twice, in the current implementation. It is undesirable.
> 
> Then, the meaning of jffs2_set_acl() should be defined again as a function
> which simply put a posix_acl object into a on-medium xattr.
> 
> In addition, some updated function got unnecessary to have an extra argument
> of acl, if posix_acl object is set onto f->i_acl_access and f->i_acl_default
> in jffs2_new_inode().
> 
> Thanks,

-- 
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai at ak.jp.nec.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jffs2_two_phase_ACL-Init.patch
Type: text/x-patch
Size: 12334 bytes
Desc: not available
Url : http://lists.infradead.org/pipermail/linux-mtd/attachments/20070914/6cc38a72/attachment-0001.bin