| Message ID | 87mvxxcogp.fsf@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 11, 2015 at 07:58:14PM -0500, Eric W. Biederman wrote: > Andy Lutomirski <luto@amacapital.net> writes: > > > On Tue, Aug 11, 2015 at 11:57 AM, Eric W. Biederman > > <ebiederm@xmission.com> wrote: > >> > >> *Boggle* > >> > >> The only time this should prevent anything is when in a container when > >> you are not global root. And then only mounting sysfs should be > >> affected. > > > > Before: > > > > open("/sys/kernel/debug/asdf", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, > > 0666) = -1 EACCES (Permission denied) > > > > > > After: > > > > open("/sys/kernel/debug/asdf", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, > > 0666) = -1 ENOENT (No such file or directory) > > > > Something broke. I don't know whether CentOS cares about that change, > > but there could be other odd side effects. > > Thanks for pointing this out. I don't know if broke is quite the right > word for a change in error codes on lookup failure, but I agree it is a > difference that could have affected something. > > The behavior of empty proc dirs actually return -ENOENT in this > situation and so it is a little fuzzy about which is the best behavior > to use. > > Creativing a negative dentry and and then letting vfs_create fail may be > the better way to go. > > Negative dentries are weird enough that I would prefer not to have code > that creates negative dentries. They could easily be a lurking trap > for some filesystems dentry operations. > > The patch below is enough to change the error code if someone who can > reproduce this wants to try this. > > Eric > > diff --gdiff --git a/fs/libfs.c b/fs/libfs.c > index 102edfd39000..3a452a485cbf 100644 > --- a/fs/libfs.c > +++ b/fs/libfs.c > @@ -1109,7 +1109,7 @@ EXPORT_SYMBOL(simple_symlink_inode_operations); > */ > static struct dentry *empty_dir_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) > { > - return ERR_PTR(-ENOENT); > + return NULL; And let's please restore this too. Sentiments about negative dentries aside, It's outright wrong to report -ENOENT on creat. Thanks.
Tejun Heo <tj@kernel.org> writes: > On Tue, Aug 11, 2015 at 07:58:14PM -0500, Eric W. Biederman wrote: >> Andy Lutomirski <luto@amacapital.net> writes: >> >> > On Tue, Aug 11, 2015 at 11:57 AM, Eric W. Biederman >> > <ebiederm@xmission.com> wrote: >> >> >> >> *Boggle* >> >> >> >> The only time this should prevent anything is when in a container when >> >> you are not global root. And then only mounting sysfs should be >> >> affected. >> > >> > Before: >> > >> > open("/sys/kernel/debug/asdf", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, >> > 0666) = -1 EACCES (Permission denied) >> > >> > >> > After: >> > >> > open("/sys/kernel/debug/asdf", O_WRONLY|O_CREAT|O_NOCTTY|O_NONBLOCK, >> > 0666) = -1 ENOENT (No such file or directory) >> > >> > Something broke. I don't know whether CentOS cares about that change, >> > but there could be other odd side effects. >> >> Thanks for pointing this out. I don't know if broke is quite the right >> word for a change in error codes on lookup failure, but I agree it is a >> difference that could have affected something. >> >> The behavior of empty proc dirs actually return -ENOENT in this >> situation and so it is a little fuzzy about which is the best behavior >> to use. >> >> Creativing a negative dentry and and then letting vfs_create fail may be >> the better way to go. >> >> Negative dentries are weird enough that I would prefer not to have code >> that creates negative dentries. They could easily be a lurking trap >> for some filesystems dentry operations. >> >> The patch below is enough to change the error code if someone who can >> reproduce this wants to try this. >> >> Eric >> >> diff --gdiff --git a/fs/libfs.c b/fs/libfs.c >> index 102edfd39000..3a452a485cbf 100644 >> --- a/fs/libfs.c >> +++ b/fs/libfs.c >> @@ -1109,7 +1109,7 @@ EXPORT_SYMBOL(simple_symlink_inode_operations); >> */ >> static struct dentry *empty_dir_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) >> { >> - return ERR_PTR(-ENOENT); >> + return NULL; > > And let's please restore this too. Sentiments about negative dentries > aside, It's outright wrong to report -ENOENT on creat. proc has always reported -ENOENT. sysfs is the odd one out. I am not completely certain that trivial patch above, does not introduce a leak, a NULL pointer dereference or something else nasty when the code is hit. So far it seems that no one cares. And since the change is brittle I am not inclined to mess with it this week, as I have other demands on my limited review bandwidth right now. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, Aug 12, 2015 at 03:27:26PM -0500, Eric W. Biederman wrote: > proc has always reported -ENOENT. sysfs is the odd one out. Hmm... open(2) is clear about failure modes and ENOENT doesn't fit the bill here. Maintaining the behavior for proc for backward compatibility is fine but I don't think it's appropriate to change behaviors on other filesystems which were behaving correctly especially through changes which got routed through -stable. ENOENT O_CREAT is not set and the named file does not exist. Or, a directory component in pathname does not exist or is a dangling symbolic link. ENOENT pathname refers to a nonexistent directory, O_TMPFILE and one of O_WRONLY or O_RDWR were specified in flags, but this kernel version does not provide the O_TMPFILE functionality. > I am not completely certain that trivial patch above, does not introduce > a leak, a NULL pointer dereference or something else nasty when the code > is hit. > > So far it seems that no one cares. And since the change is brittle I am > not inclined to mess with it this week, as I have other demands on my > limited review bandwidth right now. Sure, it isn't "today" urgent but let's please restore the original behavior before the new behavior gets too widespread. Thanks.
diff --gdiff --git a/fs/libfs.c b/fs/libfs.c index 102edfd39000..3a452a485cbf 100644 --- a/fs/libfs.c +++ b/fs/libfs.c @@ -1109,7 +1109,7 @@ EXPORT_SYMBOL(simple_symlink_inode_operations); */ static struct dentry *empty_dir_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) { - return ERR_PTR(-ENOENT); + return NULL; } static int empty_dir_getattr(struct vfsmount *mnt, struct dentry *dentry,