| Message ID | 20181001224500.GE5872@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs: check ->get_link return value | expand |
On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Teach callers of inode->i_op->get_link in the vfs code to check for a > NULL return value and return an error status instead of blindly > dereferencing the returned NULL pointer. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/namei.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/namei.c b/fs/namei.c > index 0cab6494978c..0744ab981fa0 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) > if (IS_ERR(link)) > return PTR_ERR(link); > } > + if (!link) > + return -EUCLEAN; If we are going to start returning this as a filesystem corruption error from the VFS, can we please start with adding #define EFSCORRUPTED EUCLEAN into one of the global error definition headers? The code makes much more sense when it uses EFSCORRUPTED.... Cheers, Dave.
On Tue, Oct 02, 2018 at 09:21:28AM +1000, Dave Chinner wrote: > On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Teach callers of inode->i_op->get_link in the vfs code to check for a > > NULL return value and return an error status instead of blindly > > dereferencing the returned NULL pointer. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/namei.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/fs/namei.c b/fs/namei.c > > index 0cab6494978c..0744ab981fa0 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) > > if (IS_ERR(link)) > > return PTR_ERR(link); > > } > > + if (!link) > > + return -EUCLEAN; > > If we are going to start returning this as a filesystem corruption > error from the VFS, can we please start with adding > > #define EFSCORRUPTED EUCLEAN > > into one of the global error definition headers? The code makes much > more sense when it uses EFSCORRUPTED.... Ok, which header file? include/uapi/asm-generic/errno.h ? We'll finally be able to get rid of the redundant definitions in ext[24] too. --D > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Teach callers of inode->i_op->get_link in the vfs code to check for a > NULL return value and return an error status instead of blindly > dereferencing the returned NULL pointer. Is that better than having the get_link method return ERR_PTR(-EUCLEAN) itself? > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/namei.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/namei.c b/fs/namei.c > index 0cab6494978c..0744ab981fa0 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) > if (IS_ERR(link)) > return PTR_ERR(link); > } > + if (!link) > + return -EUCLEAN; > res = readlink_copy(buffer, buflen, link); > do_delayed_call(&done); > return res; > @@ -4763,6 +4765,8 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done) > res = ERR_PTR(security_inode_readlink(dentry)); > if (!res) > res = inode->i_op->get_link(dentry, inode, done); > + if (!res) > + return ERR_PTR(-EUCLEAN); > } > return res; > }
On Mon, Oct 01, 2018 at 04:33:36PM -0700, Darrick J. Wong wrote: > On Tue, Oct 02, 2018 at 09:21:28AM +1000, Dave Chinner wrote: > > On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > Teach callers of inode->i_op->get_link in the vfs code to check for a > > > NULL return value and return an error status instead of blindly > > > dereferencing the returned NULL pointer. > > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > > --- > > > fs/namei.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/fs/namei.c b/fs/namei.c > > > index 0cab6494978c..0744ab981fa0 100644 > > > --- a/fs/namei.c > > > +++ b/fs/namei.c > > > @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) > > > if (IS_ERR(link)) > > > return PTR_ERR(link); > > > } > > > + if (!link) > > > + return -EUCLEAN; > > > > If we are going to start returning this as a filesystem corruption > > error from the VFS, can we please start with adding > > > > #define EFSCORRUPTED EUCLEAN > > > > into one of the global error definition headers? The code makes much > > more sense when it uses EFSCORRUPTED.... > > Ok, which header file? include/uapi/asm-generic/errno.h ? Seems reasonable, because that's where EUCLEAN is defined. > We'll finally be able to get rid of the redundant definitions in > ext[24] too. And filesystems like btrfs can be converted from EUCLEAN to EFSCORRUPTED, too. Cheers, Dave.
On Mon, Oct 01, 2018 at 04:52:51PM -0700, Matthew Wilcox wrote: > On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Teach callers of inode->i_op->get_link in the vfs code to check for a > > NULL return value and return an error status instead of blindly > > dereferencing the returned NULL pointer. > > Is that better than having the get_link method return ERR_PTR(-EUCLEAN) itself? get_link doesn't need the EFSCORRUPTED return; all two of its callers handle null pointer returns correctly and they don't return the ->get_link return value directly to userspace. It's just these two functions below whose callers assume they have to deal an error pointer or that it's totally safe to dereference it. --D > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > > --- > > fs/namei.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/fs/namei.c b/fs/namei.c > > index 0cab6494978c..0744ab981fa0 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) > > if (IS_ERR(link)) > > return PTR_ERR(link); > > } > > + if (!link) > > + return -EUCLEAN; > > res = readlink_copy(buffer, buflen, link); > > do_delayed_call(&done); > > return res; > > @@ -4763,6 +4765,8 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done) > > res = ERR_PTR(security_inode_readlink(dentry)); > > if (!res) > > res = inode->i_op->get_link(dentry, inode, done); > > + if (!res) > > + return ERR_PTR(-EUCLEAN); > > } > > return res; > > }
On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > Teach callers of inode->i_op->get_link in the vfs code to check for a > NULL return value and return an error status instead of blindly > dereferencing the returned NULL pointer. IDGI. If you want it to fail with -EUCLEAN, then by all means return it as you would any other error. I've no problem with "fs image is fucked, return an error". However, "fs driver is fucked, paper over that if we'd caught one of the symptoms" is a different story. NAK in that form. If we have that happen, let the damn thing oops. Quietly papering over bugs like that is just plain wrong.
On Mon, Oct 01, 2018 at 05:23:32PM -0700, Darrick J. Wong wrote: > get_link doesn't need the EFSCORRUPTED return; all two of its callers > handle null pointer returns correctly and they don't return the ->get_link > return value directly to userspace. > > It's just these two functions below whose callers assume they have to > deal an error pointer or that it's totally safe to dereference it. No. The only case when ->get_link() has any business returning NULL is when it has done nd_jump_link(). Those should never come without explicit ->readlink() and they should never be fed to vfs_get_link(), so they are not a problem; anything else is a filesystem driver bug, plain and simple. Check for NULL in fs/namei.c:get_link() is *NOT* "defensive programming" bullshit; there it can legitimately happen (aforementioned procfs-style symlinks). Note that there it is not an error at all. Current calling conventions are: * return ERR_PTR(-E...) on error * return pointer to symlink body to be traversed on success * return NULL when ->get_link() instances has jumped to destination on its own and there's no "symlink body" to be traversed. For such symlinks we obviously need an explicit ->readlink() (for whatever string we want readlink(2) to return). These should not be occur on anything NFS-exported or on overlayfs layers, since neither NFSD nor overlayfs don't know what to do with such. What you are proposing is a weird change along the lines of "if you accidentally return NULL it will be treated as empty body, except when it occurs on NFS exports or overlayfs layers; in such cases it will be interpreted as fs corruption. $DEITY help you if real procfs-style symlink hits one of those, since nd_jump_link() called by those will oops in such conditions". As a mitigation strategy it sucks. As part of calling conventions it's confusing and AFAICS absolutely pointless. NAK. And I'm really curious - what has lead to that? Because procfs-style symlink in such conditions would have oopsed before it returned from ->get_link()...
On Tue, Oct 02, 2018 at 02:31:06AM +0100, Al Viro wrote: > On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > Teach callers of inode->i_op->get_link in the vfs code to check for a > > NULL return value and return an error status instead of blindly > > dereferencing the returned NULL pointer. > > IDGI. If you want it to fail with -EUCLEAN, then by all means return > it as you would any other error. > > I've no problem with "fs image is fucked, return an error". However, > "fs driver is fucked, paper over that if we'd caught one of the > symptoms" is a different story. This whole thread got started from a suggestion Christoph made about a patch I had to fix the XFS side to return an error instead of a null pointer: https://www.spinics.net/lists/linux-xfs/msg21372.html I hadn't thought it was all /that/ necessary to fix the vfs since there are plenty of other places where the vfs assumes the fs knows what it's doing and sneezes hard if not... > NAK in that form. If we have that happen, let the damn thing oops. > Quietly papering over bugs like that is just plain wrong. Sounds good to me! Patch withdrawn. :) --D
On Mon, Oct 01, 2018 at 07:07:12PM -0700, Darrick J. Wong wrote: > On Tue, Oct 02, 2018 at 02:31:06AM +0100, Al Viro wrote: > > On Mon, Oct 01, 2018 at 03:45:00PM -0700, Darrick J. Wong wrote: > > > From: Darrick J. Wong <darrick.wong@oracle.com> > > > > > > Teach callers of inode->i_op->get_link in the vfs code to check for a > > > NULL return value and return an error status instead of blindly > > > dereferencing the returned NULL pointer. > > > > IDGI. If you want it to fail with -EUCLEAN, then by all means return > > it as you would any other error. > > > > I've no problem with "fs image is fucked, return an error". However, > > "fs driver is fucked, paper over that if we'd caught one of the > > symptoms" is a different story. > > This whole thread got started from a suggestion Christoph made about a > patch I had to fix the XFS side to return an error instead of a null > pointer: > > https://www.spinics.net/lists/linux-xfs/msg21372.html Ugh... What should happen for that to trigger? If anything, I would rather validate that somewhere around xfs_setup_iops() *AND* set ->i_link at the same time, killing the whole xfs_vn_get_link_inline() thing (just use simple_get_link() instead)... See another reply for the reasons why such mitigation makes no sense.
diff --git a/fs/namei.c b/fs/namei.c index 0cab6494978c..0744ab981fa0 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -4737,6 +4737,8 @@ int vfs_readlink(struct dentry *dentry, char __user *buffer, int buflen) if (IS_ERR(link)) return PTR_ERR(link); } + if (!link) + return -EUCLEAN; res = readlink_copy(buffer, buflen, link); do_delayed_call(&done); return res; @@ -4763,6 +4765,8 @@ const char *vfs_get_link(struct dentry *dentry, struct delayed_call *done) res = ERR_PTR(security_inode_readlink(dentry)); if (!res) res = inode->i_op->get_link(dentry, inode, done); + if (!res) + return ERR_PTR(-EUCLEAN); } return res; }