linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vfs: check ->get_link return value
@ 2018-10-01 22:45 Darrick J. Wong
  2018-10-01 23:21 ` Dave Chinner
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Darrick J. Wong @ 2018-10-01 22:45 UTC (permalink / raw)
  To: viro; +Cc: xfs, linux-fsdevel, Christoph Hellwig

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;
 	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;
 }

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-01 22:45 [PATCH] vfs: check ->get_link return value Darrick J. Wong
@ 2018-10-01 23:21 ` Dave Chinner
  2018-10-01 23:33   ` Darrick J. Wong
  2018-10-01 23:52 ` Matthew Wilcox
  2018-10-02  1:31 ` Al Viro
  2 siblings, 1 reply; 10+ messages in thread
From: Dave Chinner @ 2018-10-01 23:21 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: viro, xfs, linux-fsdevel, Christoph Hellwig

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.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-01 23:21 ` Dave Chinner
@ 2018-10-01 23:33   ` Darrick J. Wong
  2018-10-01 23:53     ` Dave Chinner
  0 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2018-10-01 23:33 UTC (permalink / raw)
  To: Dave Chinner; +Cc: viro, xfs, linux-fsdevel, Christoph Hellwig

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-01 22:45 [PATCH] vfs: check ->get_link return value Darrick J. Wong
  2018-10-01 23:21 ` Dave Chinner
@ 2018-10-01 23:52 ` Matthew Wilcox
  2018-10-02  0:23   ` Darrick J. Wong
  2018-10-02  1:31 ` Al Viro
  2 siblings, 1 reply; 10+ messages in thread
From: Matthew Wilcox @ 2018-10-01 23:52 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: viro, xfs, linux-fsdevel, Christoph Hellwig

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;
>  }

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-01 23:33   ` Darrick J. Wong
@ 2018-10-01 23:53     ` Dave Chinner
  0 siblings, 0 replies; 10+ messages in thread
From: Dave Chinner @ 2018-10-01 23:53 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: viro, xfs, linux-fsdevel, Christoph Hellwig

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.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-01 23:52 ` Matthew Wilcox
@ 2018-10-02  0:23   ` Darrick J. Wong
  2018-10-02  1:54     ` Al Viro
  0 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2018-10-02  0:23 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: viro, xfs, linux-fsdevel, Christoph Hellwig

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;
> >  }

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-01 22:45 [PATCH] vfs: check ->get_link return value Darrick J. Wong
  2018-10-01 23:21 ` Dave Chinner
  2018-10-01 23:52 ` Matthew Wilcox
@ 2018-10-02  1:31 ` Al Viro
  2018-10-02  2:07   ` Darrick J. Wong
  2 siblings, 1 reply; 10+ messages in thread
From: Al Viro @ 2018-10-02  1:31 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: xfs, linux-fsdevel, Christoph Hellwig

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-02  0:23   ` Darrick J. Wong
@ 2018-10-02  1:54     ` Al Viro
  0 siblings, 0 replies; 10+ messages in thread
From: Al Viro @ 2018-10-02  1:54 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Matthew Wilcox, xfs, linux-fsdevel, Christoph Hellwig

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()...

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-02  1:31 ` Al Viro
@ 2018-10-02  2:07   ` Darrick J. Wong
  2018-10-02  2:47     ` Al Viro
  0 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2018-10-02  2:07 UTC (permalink / raw)
  To: Al Viro; +Cc: xfs, linux-fsdevel, Christoph Hellwig

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] vfs: check ->get_link return value
  2018-10-02  2:07   ` Darrick J. Wong
@ 2018-10-02  2:47     ` Al Viro
  0 siblings, 0 replies; 10+ messages in thread
From: Al Viro @ 2018-10-02  2:47 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: xfs, linux-fsdevel, Christoph Hellwig

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-10-02  9:27 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-01 22:45 [PATCH] vfs: check ->get_link return value Darrick J. Wong
2018-10-01 23:21 ` Dave Chinner
2018-10-01 23:33   ` Darrick J. Wong
2018-10-01 23:53     ` Dave Chinner
2018-10-01 23:52 ` Matthew Wilcox
2018-10-02  0:23   ` Darrick J. Wong
2018-10-02  1:54     ` Al Viro
2018-10-02  1:31 ` Al Viro
2018-10-02  2:07   ` Darrick J. Wong
2018-10-02  2:47     ` Al Viro

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).