Re: [PATCH] NFS: fix BUG() crash in notify_change() with patch to chown_common()

Re: [PATCH] NFS: fix BUG() crash in notify_change() with patch to chown_common()

[J. Bruce Fields]

Looks OK to me.  I think it would go through Al.--b.

Acked-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

On Mon, Feb 23, 2015 at 08:51:24AM -0500, Andrew Elble wrote:
> We have observed a BUG() crash in fs/attr.c:notify_change(). The crash
> occurs during an rsync into a filesystem that is exported via NFS.
> 
> 1.) fs/attr.c:notify_change() modifies the caller's version of attr.
> 2.) 6de0ec00ba8d ("VFS: make notify_change pass ATTR_KILL_S*ID to
>     setattr operations") introduced a BUG() restriction such that "no
>     function will ever call notify_change() with both ATTR_MODE and
>     ATTR_KILL_S*ID set". Under some circumstances though, it will have
>     assisted in setting the caller's version of attr to this very
>     combination.
> 3.) 27ac0ffeac80 ("locks: break delegations on any attribute
>     modification") introduced code to handle breaking
>     delegations. This can result in notify_change() being re-called. attr
>     _must_ be explicitly reset to avoid triggering the BUG() established
>     in #2.
> 4.) The path that that triggers this is via fs/open.c:chmod_common().
>     The combination of attr flags set here and in the first call to
>     notify_change() along with a later failed break_deleg_wait()
>     results in notify_change() being called again via retry_deleg
>     without resetting attr.
> 
> Solution is to move retry_deleg in chmod_common() a bit further up to
> ensure attr is completely reset.
> 
> There are other places where this seemingly could occur, such as
> fs/utimes.c:utimes_common(), but the attr flags are not initially
> set in such a way to trigger this.
> 
> Fixes: 27ac0ffeac80 ("locks: break delegations on any attribute modification")
> Reported-by: Eric Meddaugh <etmsys-H6Ufl4FQnQQ@public.gmane.org>
> Tested-by: Eric Meddaugh <etmsys-H6Ufl4FQnQQ@public.gmane.org>
> Signed-off-by: Andrew Elble <aweits-H6Ufl4FQnQQ@public.gmane.org>
> ---
>  fs/open.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 33f9cbf2610b..44a3be145bfe 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -570,6 +570,7 @@ static int chown_common(struct path *path, uid_t user, gid_t group)
>  	uid = make_kuid(current_user_ns(), user);
>  	gid = make_kgid(current_user_ns(), group);
>  
> +retry_deleg:
>  	newattrs.ia_valid =  ATTR_CTIME;
>  	if (user != (uid_t) -1) {
>  		if (!uid_valid(uid))
> @@ -586,7 +587,6 @@ static int chown_common(struct path *path, uid_t user, gid_t group)
>  	if (!S_ISDIR(inode->i_mode))
>  		newattrs.ia_valid |=
>  			ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> -retry_deleg:
>  	mutex_lock(&inode->i_mutex);
>  	error = security_path_chown(path, uid, gid);
>  	if (!error)
> -- 
> 1.9.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] NFS: fix BUG() crash in notify_change() with patch to chown_common()

[Andrew W Elble]

Any update/feedback?

"J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> writes:

> Looks OK to me.  I think it would go through Al.--b.
>
> Acked-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> On Mon, Feb 23, 2015 at 08:51:24AM -0500, Andrew Elble wrote:
>> We have observed a BUG() crash in fs/attr.c:notify_change(). The crash
>> occurs during an rsync into a filesystem that is exported via NFS.
>> 
>> 1.) fs/attr.c:notify_change() modifies the caller's version of attr.
>> 2.) 6de0ec00ba8d ("VFS: make notify_change pass ATTR_KILL_S*ID to
>>     setattr operations") introduced a BUG() restriction such that "no
>>     function will ever call notify_change() with both ATTR_MODE and
>>     ATTR_KILL_S*ID set". Under some circumstances though, it will have
>>     assisted in setting the caller's version of attr to this very
>>     combination.
>> 3.) 27ac0ffeac80 ("locks: break delegations on any attribute
>>     modification") introduced code to handle breaking
>>     delegations. This can result in notify_change() being re-called. attr
>>     _must_ be explicitly reset to avoid triggering the BUG() established
>>     in #2.
>> 4.) The path that that triggers this is via fs/open.c:chmod_common().
>>     The combination of attr flags set here and in the first call to
>>     notify_change() along with a later failed break_deleg_wait()
>>     results in notify_change() being called again via retry_deleg
>>     without resetting attr.
>> 
>> Solution is to move retry_deleg in chmod_common() a bit further up to
>> ensure attr is completely reset.
>> 
>> There are other places where this seemingly could occur, such as
>> fs/utimes.c:utimes_common(), but the attr flags are not initially
>> set in such a way to trigger this.
>> 
>> Fixes: 27ac0ffeac80 ("locks: break delegations on any attribute modification")
>> Reported-by: Eric Meddaugh <etmsys-H6Ufl4FQnQQ@public.gmane.org>
>> Tested-by: Eric Meddaugh <etmsys-H6Ufl4FQnQQ@public.gmane.org>
>> Signed-off-by: Andrew Elble <aweits-H6Ufl4FQnQQ@public.gmane.org>
>> ---
>>  fs/open.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/open.c b/fs/open.c
>> index 33f9cbf2610b..44a3be145bfe 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -570,6 +570,7 @@ static int chown_common(struct path *path, uid_t user, gid_t group)
>>  	uid = make_kuid(current_user_ns(), user);
>>  	gid = make_kgid(current_user_ns(), group);
>>  
>> +retry_deleg:
>>  	newattrs.ia_valid =  ATTR_CTIME;
>>  	if (user != (uid_t) -1) {
>>  		if (!uid_valid(uid))
>> @@ -586,7 +587,6 @@ static int chown_common(struct path *path, uid_t user, gid_t group)
>>  	if (!S_ISDIR(inode->i_mode))
>>  		newattrs.ia_valid |=
>>  			ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
>> -retry_deleg:
>>  	mutex_lock(&inode->i_mutex);
>>  	error = security_path_chown(path, uid, gid);
>>  	if (!error)
>> -- 
>> 1.9.2
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Andrew W. Elble
aweits-AzKTfQcKJ+UHTWTJ7q3cEje48wsgrGvP@public.gmane.org
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] NFS: fix BUG() crash in notify_change() with patch to chown_common()

[J. Bruce Fields]

On Fri, Mar 20, 2015 at 10:28:31AM -0400, Andrew W Elble wrote:
> 
> Any update/feedback?

If you haven't already: could you resend it, To: Al, and a "Cc:
stable@vger.kernel.org" line?

--b.

> 
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> > Looks OK to me.  I think it would go through Al.--b.
> >
> > Acked-by: J. Bruce Fields <bfields@redhat.com>
> >
> > On Mon, Feb 23, 2015 at 08:51:24AM -0500, Andrew Elble wrote:
> >> We have observed a BUG() crash in fs/attr.c:notify_change(). The crash
> >> occurs during an rsync into a filesystem that is exported via NFS.
> >> 
> >> 1.) fs/attr.c:notify_change() modifies the caller's version of attr.
> >> 2.) 6de0ec00ba8d ("VFS: make notify_change pass ATTR_KILL_S*ID to
> >>     setattr operations") introduced a BUG() restriction such that "no
> >>     function will ever call notify_change() with both ATTR_MODE and
> >>     ATTR_KILL_S*ID set". Under some circumstances though, it will have
> >>     assisted in setting the caller's version of attr to this very
> >>     combination.
> >> 3.) 27ac0ffeac80 ("locks: break delegations on any attribute
> >>     modification") introduced code to handle breaking
> >>     delegations. This can result in notify_change() being re-called. attr
> >>     _must_ be explicitly reset to avoid triggering the BUG() established
> >>     in #2.
> >> 4.) The path that that triggers this is via fs/open.c:chmod_common().
> >>     The combination of attr flags set here and in the first call to
> >>     notify_change() along with a later failed break_deleg_wait()
> >>     results in notify_change() being called again via retry_deleg
> >>     without resetting attr.
> >> 
> >> Solution is to move retry_deleg in chmod_common() a bit further up to
> >> ensure attr is completely reset.
> >> 
> >> There are other places where this seemingly could occur, such as
> >> fs/utimes.c:utimes_common(), but the attr flags are not initially
> >> set in such a way to trigger this.
> >> 
> >> Fixes: 27ac0ffeac80 ("locks: break delegations on any attribute modification")
> >> Reported-by: Eric Meddaugh <etmsys@rit.edu>
> >> Tested-by: Eric Meddaugh <etmsys@rit.edu>
> >> Signed-off-by: Andrew Elble <aweits@rit.edu>
> >> ---
> >>  fs/open.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/fs/open.c b/fs/open.c
> >> index 33f9cbf2610b..44a3be145bfe 100644
> >> --- a/fs/open.c
> >> +++ b/fs/open.c
> >> @@ -570,6 +570,7 @@ static int chown_common(struct path *path, uid_t user, gid_t group)
> >>  	uid = make_kuid(current_user_ns(), user);
> >>  	gid = make_kgid(current_user_ns(), group);
> >>  
> >> +retry_deleg:
> >>  	newattrs.ia_valid =  ATTR_CTIME;
> >>  	if (user != (uid_t) -1) {
> >>  		if (!uid_valid(uid))
> >> @@ -586,7 +587,6 @@ static int chown_common(struct path *path, uid_t user, gid_t group)
> >>  	if (!S_ISDIR(inode->i_mode))
> >>  		newattrs.ia_valid |=
> >>  			ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
> >> -retry_deleg:
> >>  	mutex_lock(&inode->i_mutex);
> >>  	error = security_path_chown(path, uid, gid);
> >>  	if (!error)
> >> -- 
> >> 1.9.2
> >> 
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> -- 
> Andrew W. Elble
> aweits@discipline.rit.edu
> Infrastructure Engineer, Communications Technical Lead
> Rochester Institute of Technology
> PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912

Re: [PATCH] NFS: fix BUG() crash in notify_change() with patch to chown_common()

[Al Viro]

On Fri, Mar 20, 2015 at 10:50:13AM -0400, J. Bruce Fields wrote:
> On Fri, Mar 20, 2015 at 10:28:31AM -0400, Andrew W Elble wrote:
> > 
> > Any update/feedback?
> 
> If you haven't already: could you resend it, To: Al, and a "Cc:
> stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" line?

It's in the local queue; will push when I reorder it tonight...
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] NFS: fix BUG() crash in notify_change() with patch to chown_common()

[J. Bruce Fields]

On Fri, Mar 20, 2015 at 02:53:51PM +0000, Al Viro wrote:
> On Fri, Mar 20, 2015 at 10:50:13AM -0400, J. Bruce Fields wrote:
> > On Fri, Mar 20, 2015 at 10:28:31AM -0400, Andrew W Elble wrote:
> > > 
> > > Any update/feedback?
> > 
> > If you haven't already: could you resend it, To: Al, and a "Cc:
> > stable@vger.kernel.org" line?
> 
> It's in the local queue; will push when I reorder it tonight...

Thanks!

--b.

Re: [PATCH] NFS: fix BUG() crash in notify_change() with patch to chown_common()

[Andrew W Elble]

>> It's in the local queue; will push when I reorder it tonight...
>
> Thanks!
>
> --b.
>

Thank you!

-- 
Andrew W. Elble
aweits@discipline.rit.edu
Infrastructure Engineer, Communications Technical Lead
Rochester Institute of Technology
PGP: BFAD 8461 4CCF DC95 DA2C B0EB 965B 082E 863E C912

[PATCH] NFS: fix BUG() crash in notify_change() with patch to chown_common()

[Andrew Elble]

We have observed a BUG() crash in fs/attr.c:notify_change(). The crash
occurs during an rsync into a filesystem that is exported via NFS.

1.) fs/attr.c:notify_change() modifies the caller's version of attr.
2.) 6de0ec00ba8d ("VFS: make notify_change pass ATTR_KILL_S*ID to
    setattr operations") introduced a BUG() restriction such that "no
    function will ever call notify_change() with both ATTR_MODE and
    ATTR_KILL_S*ID set". Under some circumstances though, it will have
    assisted in setting the caller's version of attr to this very
    combination.
3.) 27ac0ffeac80 ("locks: break delegations on any attribute
    modification") introduced code to handle breaking
    delegations. This can result in notify_change() being re-called. attr
    _must_ be explicitly reset to avoid triggering the BUG() established
    in #2.
4.) The path that that triggers this is via fs/open.c:chmod_common().
    The combination of attr flags set here and in the first call to
    notify_change() along with a later failed break_deleg_wait()
    results in notify_change() being called again via retry_deleg
    without resetting attr.

Solution is to move retry_deleg in chmod_common() a bit further up to
ensure attr is completely reset.

There are other places where this seemingly could occur, such as
fs/utimes.c:utimes_common(), but the attr flags are not initially
set in such a way to trigger this.

Fixes: 27ac0ffeac80 ("locks: break delegations on any attribute modification")
Reported-by: Eric Meddaugh <etmsys@rit.edu>
Tested-by: Eric Meddaugh <etmsys@rit.edu>
Signed-off-by: Andrew Elble <aweits@rit.edu>
Cc: stable@vger.kernel.org
Acked-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/open.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/open.c b/fs/open.c
index 33f9cbf2610b..44a3be145bfe 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -570,6 +570,7 @@ static int chown_common(struct path *path, uid_t user, gid_t group)
 	uid = make_kuid(current_user_ns(), user);
 	gid = make_kgid(current_user_ns(), group);
 
+retry_deleg:
 	newattrs.ia_valid =  ATTR_CTIME;
 	if (user != (uid_t) -1) {
 		if (!uid_valid(uid))
@@ -586,7 +587,6 @@ static int chown_common(struct path *path, uid_t user, gid_t group)
 	if (!S_ISDIR(inode->i_mode))
 		newattrs.ia_valid |=
 			ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
-retry_deleg:
 	mutex_lock(&inode->i_mutex);
 	error = security_path_chown(path, uid, gid);
 	if (!error)
-- 
1.9.2