| Message ID | 1467870827-2959489-1-git-send-email-green@linuxhacker.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2016-07-07 at 01:53 -0400, Oleg Drokin wrote: > (sorry for resend, the first go around did not make it to fsdevel and to Al). > > This is inspired by a bug in Lustre that's ATM is shared by NFS > and used o be shared by CIFS code. > > The problem at hand is: when you try to mkdir in a directory > where you do not have permissions to create anything, you only > supposed to get EPERM if the directory you are creatign does not exist. > Now if the name does exist, you are supposed to get EEXIST instead. > There are tons of programs that when fed a pathname go and try > to perform a create of every path component starting from /, > and ignoring EEXIST, but not other errors. Those programs are broken > by the above mentioned bug. > > All is fine everywhere by Lustre and NFS at the moment, because > there's an optimization at hand. e.g. in NFS: > /* > * If we're doing an exclusive create, optimize away the lookup > * but don't hash the dentry. > */ > if (nfs_is_exclusive_create(dir, flags)) > return NULL; > > Now, this is all fine except when you have no permissions to create > anything - then vfs_mknod/mkdir/create will do may_create(dir, dentry) > and we exit spuriously with EPERM. > > [green@fedora1 crash]$ mkdir aaa > mkdir: cannot create directory 'aaa': Permission denied > [green@fedora1 crash]$ mkdir lost+found > mkdir: cannot create directory 'lost+found': Permission denied > [green@fedora1 crash]$ ls -ld lost+found > drwx------ 2 root root 16384 May 25 2013 lost+found > [green@fedora1 crash]$ mkdir lost+found > mkdir: cannot create directory 'lost+found': File exists > > cifs had exactly the same code, but it got removed when atomic_open > was introduced (throwing away a perfectly good optimization for mkdir > in process) with commit d2c127197dfc0b2bae62a52e1e0d3e3ff493919e > "cifs: implement i_op->atomic_open()" > > These two patches are the lazy way of fixing the problem - > "just throw in the extra permission check before bailing out" > with a bit of complication on the NFS side because there > the inode permission check is actually circumvented in nfs_permission, > for MAY_WRITE | !MAY_READ case which is enough to fool > may_create, but not enough to fool some following check, I guess > as the problem still exists. > (I am not sure of the performance implications of just removing that > thing in nfs_permission). > > Anyway I think instead of resurrecting this optimization for cifs, > and seeing if ceph and others need it, why not bring it up > all the way to __lookup_hash() so that we don't do actual lookup > if the parent is writeable? > > Even for local filesystems like ext4 that's of benefit - we save > one lookup (even with hashed dirs, that only gives us the last blook > to lookat and then we still need to check all names to make sure > the one we want does not exist - so it's not exactly free). > > This should not upset any sort of client-side SELinux/other security > stuff magic either. If the name exists, we get EEXIST no matter what, > if it does not exist, parent policy declares if we can create or not > anyway. > > Something like this (+ whatever nfs_permission fix): > diff --git a/fs/namei.c b/fs/namei.c > index 70580ab..b9de645 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -1512,6 +1512,10 @@ static struct dentry *__lookup_hash(const struct qstr *name, > if (unlikely(!dentry)) > return ERR_PTR(-ENOMEM); > > + if ((flags & LOOKUP_EXCL|LOOKUP_CREATE) && > + (may_create(base, dentry) == 0)) > + return dentry; > + That would need to check that LOOKUP_EXCL is actually set. I think you want something like: (flags & (LOOKUP_EXCL|LOOKUP_CREATE)) == (LOOKUP_EXCL|LOOKUP_CREATE) ...and you'd have to figure out how to determine the isdir param for may_create at that point. That said, it does seem like a reasonable idea at first glance. > return lookup_real(base->d_inode, dentry, flags); > } > > Comments? > > Oleg Drokin (2): > nfs: Fix spurios EPERM when mkdir of existing dentry > staging/lustre: Prevent spurious EPERM on mkdir > > drivers/staging/lustre/lustre/llite/namei.c | 8 ++++++-- > fs/nfs/dir.c | 4 +++- > 2 files changed, 9 insertions(+), 3 deletions(-) >
diff --git a/fs/namei.c b/fs/namei.c index 70580ab..b9de645 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1512,6 +1512,10 @@ static struct dentry *__lookup_hash(const struct qstr *name, if (unlikely(!dentry)) return ERR_PTR(-ENOMEM); + if ((flags & LOOKUP_EXCL|LOOKUP_CREATE) && + (may_create(base, dentry) == 0)) + return dentry; + return lookup_real(base->d_inode, dentry, flags); }
(sorry for resend, the first go around did not make it to fsdevel and to Al). This is inspired by a bug in Lustre that's ATM is shared by NFS and used o be shared by CIFS code. The problem at hand is: when you try to mkdir in a directory where you do not have permissions to create anything, you only supposed to get EPERM if the directory you are creatign does not exist. Now if the name does exist, you are supposed to get EEXIST instead. There are tons of programs that when fed a pathname go and try to perform a create of every path component starting from /, and ignoring EEXIST, but not other errors. Those programs are broken by the above mentioned bug. All is fine everywhere by Lustre and NFS at the moment, because there's an optimization at hand. e.g. in NFS: /* * If we're doing an exclusive create, optimize away the lookup * but don't hash the dentry. */ if (nfs_is_exclusive_create(dir, flags)) return NULL; Now, this is all fine except when you have no permissions to create anything - then vfs_mknod/mkdir/create will do may_create(dir, dentry) and we exit spuriously with EPERM. [green@fedora1 crash]$ mkdir aaa mkdir: cannot create directory 'aaa': Permission denied [green@fedora1 crash]$ mkdir lost+found mkdir: cannot create directory 'lost+found': Permission denied [green@fedora1 crash]$ ls -ld lost+found drwx------ 2 root root 16384 May 25 2013 lost+found [green@fedora1 crash]$ mkdir lost+found mkdir: cannot create directory 'lost+found': File exists cifs had exactly the same code, but it got removed when atomic_open was introduced (throwing away a perfectly good optimization for mkdir in process) with commit d2c127197dfc0b2bae62a52e1e0d3e3ff493919e "cifs: implement i_op->atomic_open()" These two patches are the lazy way of fixing the problem - "just throw in the extra permission check before bailing out" with a bit of complication on the NFS side because there the inode permission check is actually circumvented in nfs_permission, for MAY_WRITE | !MAY_READ case which is enough to fool may_create, but not enough to fool some following check, I guess as the problem still exists. (I am not sure of the performance implications of just removing that thing in nfs_permission). Anyway I think instead of resurrecting this optimization for cifs, and seeing if ceph and others need it, why not bring it up all the way to __lookup_hash() so that we don't do actual lookup if the parent is writeable? Even for local filesystems like ext4 that's of benefit - we save one lookup (even with hashed dirs, that only gives us the last blook to lookat and then we still need to check all names to make sure the one we want does not exist - so it's not exactly free). This should not upset any sort of client-side SELinux/other security stuff magic either. If the name exists, we get EEXIST no matter what, if it does not exist, parent policy declares if we can create or not anyway. Something like this (+ whatever nfs_permission fix): Comments? Oleg Drokin (2): nfs: Fix spurios EPERM when mkdir of existing dentry staging/lustre: Prevent spurious EPERM on mkdir drivers/staging/lustre/lustre/llite/namei.c | 8 ++++++-- fs/nfs/dir.c | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-)