notify_change can modify the iattr structure. In particular it can can
end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
BUG() if the same iattr is passed to notify_change more than once.
Make a copy of the struct iattr before calling notify_change.
Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
Reported-by: Zhi Li <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/vfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index c4ef24c5ffd0..ad0c5cd900b1 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
inode_lock(inode);
for (retries = 1;;) {
- host_err = __nfsd_setattr(dentry, iap);
+ struct iattr attrs = *iap;
+
+ host_err = __nfsd_setattr(dentry, &attrs);
if (host_err != -EAGAIN || !retries--)
break;
if (!nfsd_wait_for_delegreturn(rqstp, inode))
--
2.40.1
> On May 17, 2023, at 12:26 PM, Jeff Layton <[email protected]> wrote:
>
> notify_change can modify the iattr structure. In particular it can can
> end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
> BUG() if the same iattr is passed to notify_change more than once.
>
> Make a copy of the struct iattr before calling notify_change.
>
> Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
> Reported-by: Zhi Li <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/vfs.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index c4ef24c5ffd0..ad0c5cd900b1 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>
> inode_lock(inode);
> for (retries = 1;;) {
> - host_err = __nfsd_setattr(dentry, iap);
> + struct iattr attrs = *iap;
This construct always makes me queazy. I'm never sure if an
initializer inside a loop is "only once" or "every time". I
fixed a bug like this once.
But if you've tested it and it addresses the BUG, then let's
go with this. I can apply it to nfsd-fixes.
> +
> + host_err = __nfsd_setattr(dentry, &attrs);
> if (host_err != -EAGAIN || !retries--)
> break;
> if (!nfsd_wait_for_delegreturn(rqstp, inode))
> --
> 2.40.1
>
--
Chuck Lever
> On May 17, 2023, at 3:05 PM, Jeff Layton <[email protected]> wrote:
>
> On Wed, 2023-05-17 at 17:47 +0000, Chuck Lever III wrote:
>>
>>> On May 17, 2023, at 12:26 PM, Jeff Layton <[email protected]> wrote:
>>>
>>> notify_change can modify the iattr structure. In particular it can can
>>> end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
>>> BUG() if the same iattr is passed to notify_change more than once.
>>>
>>> Make a copy of the struct iattr before calling notify_change.
>>>
>>> Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
>>> Reported-by: Zhi Li <[email protected]>
>>> Signed-off-by: Jeff Layton <[email protected]>
>>> ---
>>> fs/nfsd/vfs.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
>>> index c4ef24c5ffd0..ad0c5cd900b1 100644
>>> --- a/fs/nfsd/vfs.c
>>> +++ b/fs/nfsd/vfs.c
>>> @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>>>
>>> inode_lock(inode);
>>> for (retries = 1;;) {
>>> - host_err = __nfsd_setattr(dentry, iap);
>>> + struct iattr attrs = *iap;
>>
>> This construct always makes me queazy. I'm never sure if an
>> initializer inside a loop is "only once" or "every time". I
>> fixed a bug like this once.
>>
>> But if you've tested it and it addresses the BUG, then let's
>> go with this. I can apply it to nfsd-fixes.
>>
>
>
> I've done some light testing with this kernel, but this was found by Zhi
> while testing with the lustre racer test, so it involves some raciness.
> I've never hit this myself.
Has Zhi tested this fix?
> I'm pretty sure though that this has to be initialized every time. The
> assignment is inside the loop, after all. I'm ok with moving the
> assignment to a different line if you like though:
>
> struct iattr attrs;
>
> attrs = *iap;
> ...
Yeah I could do that. I find that easier to read when a loop is
involved; it's unambiguous then what is going on.
>>> +
>>> + host_err = __nfsd_setattr(dentry, &attrs);
>>> if (host_err != -EAGAIN || !retries--)
>>> break;
>>> if (!nfsd_wait_for_delegreturn(rqstp, inode))
>>> --
>>> 2.40.1
>>>
>>
>> --
>> Chuck Lever
>>
>>
>
> --
> Jeff Layton <[email protected]>
--
Chuck Lever
On Wed, 2023-05-17 at 12:26 -0400, Jeff Layton wrote:
> notify_change can modify the iattr structure. In particular it can can
> end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
> BUG() if the same iattr is passed to notify_change more than once.
>
> Make a copy of the struct iattr before calling notify_change.
>
> Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
> Reported-by: Zhi Li <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/vfs.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index c4ef24c5ffd0..ad0c5cd900b1 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>
> inode_lock(inode);
> for (retries = 1;;) {
> - host_err = __nfsd_setattr(dentry, iap);
> + struct iattr attrs = *iap;
> +
> + host_err = __nfsd_setattr(dentry, &attrs);
> if (host_err != -EAGAIN || !retries--)
> break;
> if (!nfsd_wait_for_delegreturn(rqstp, inode))
Zhi Li tested the test kernel for this today and this seems to have
fixed the issue. I think you can add:
Tested-by: Zhi Li <[email protected]>
Cheers,
Jeff
On Thu, 18 May 2023, Jeff Layton wrote:
> notify_change can modify the iattr structure. In particular it can can
> end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
> BUG() if the same iattr is passed to notify_change more than once.
>
> Make a copy of the struct iattr before calling notify_change.
>
> Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
> Reported-by: Zhi Li <[email protected]>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/vfs.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index c4ef24c5ffd0..ad0c5cd900b1 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
>
> inode_lock(inode);
> for (retries = 1;;) {
> - host_err = __nfsd_setattr(dentry, iap);
> + struct iattr attrs = *iap;
> +
> + host_err = __nfsd_setattr(dentry, &attrs);
I think this needs something to ensure a well meaning by-passer doesn't
try to "optimise" it back to the way it was.
Maybe make "iap" const? Or add a comment? Or both?
NeilBrown
> if (host_err != -EAGAIN || !retries--)
> break;
> if (!nfsd_wait_for_delegreturn(rqstp, inode))
> --
> 2.40.1
>
>
On Mon, 2023-05-22 at 12:45 +1000, NeilBrown wrote:
> On Thu, 18 May 2023, Jeff Layton wrote:
> > notify_change can modify the iattr structure. In particular it can can
> > end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
> > BUG() if the same iattr is passed to notify_change more than once.
> >
> > Make a copy of the struct iattr before calling notify_change.
> >
> > Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
> > Reported-by: Zhi Li <[email protected]>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/vfs.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index c4ef24c5ffd0..ad0c5cd900b1 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> >
> > inode_lock(inode);
> > for (retries = 1;;) {
> > - host_err = __nfsd_setattr(dentry, iap);
> > + struct iattr attrs = *iap;
> > +
> > + host_err = __nfsd_setattr(dentry, &attrs);
>
> I think this needs something to ensure a well meaning by-passer doesn't
> try to "optimise" it back to the way it was.
> Maybe make "iap" const? Or add a comment? Or both?
>
We can't make iap const, as we have to call nfsd_sanitize_attrs on it,
and that will change things in it. I think we'll probably have to settle
for a comment. Chuck, can we fold the patch below in to this one?
--------------------8<------------------
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 2a3687cdf926..817effd63730 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -538,6 +538,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
for (retries = 1;;) {
struct iattr attrs;
+ /*
+ * notify_change can alter the iattr in ways that make it
+ * unsuitable for submission multiple times. Make a copy
+ * for every loop.
+ */
attrs = *iap;
host_err = __nfsd_setattr(dentry, &attrs);
if (host_err != -EAGAIN || !retries--)
On Tue, May 23, 2023 at 09:41:14AM -0400, Jeff Layton wrote:
> On Mon, 2023-05-22 at 12:45 +1000, NeilBrown wrote:
> > On Thu, 18 May 2023, Jeff Layton wrote:
> > > notify_change can modify the iattr structure. In particular it can can
> > > end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
> > > BUG() if the same iattr is passed to notify_change more than once.
> > >
> > > Make a copy of the struct iattr before calling notify_change.
> > >
> > > Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
> > > Reported-by: Zhi Li <[email protected]>
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > ---
> > > fs/nfsd/vfs.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > index c4ef24c5ffd0..ad0c5cd900b1 100644
> > > --- a/fs/nfsd/vfs.c
> > > +++ b/fs/nfsd/vfs.c
> > > @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > >
> > > inode_lock(inode);
> > > for (retries = 1;;) {
> > > - host_err = __nfsd_setattr(dentry, iap);
> > > + struct iattr attrs = *iap;
> > > +
> > > + host_err = __nfsd_setattr(dentry, &attrs);
> >
> > I think this needs something to ensure a well meaning by-passer doesn't
> > try to "optimise" it back to the way it was.
> > Maybe make "iap" const? Or add a comment? Or both?
> >
>
> We can't make iap const, as we have to call nfsd_sanitize_attrs on it,
> and that will change things in it. I think we'll probably have to settle
> for a comment. Chuck, can we fold the patch below in to this one?
Done, and pushed to nfsd-fixes.
> --------------------8<------------------
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 2a3687cdf926..817effd63730 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -538,6 +538,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> for (retries = 1;;) {
> struct iattr attrs;
>
> + /*
> + * notify_change can alter the iattr in ways that make it
> + * unsuitable for submission multiple times. Make a copy
> + * for every loop.
> + */
> attrs = *iap;
> host_err = __nfsd_setattr(dentry, &attrs);
> if (host_err != -EAGAIN || !retries--)
>
On Tue, 23 May 2023, Jeff Layton wrote:
> On Mon, 2023-05-22 at 12:45 +1000, NeilBrown wrote:
> > On Thu, 18 May 2023, Jeff Layton wrote:
> > > notify_change can modify the iattr structure. In particular it can can
> > > end up setting ATTR_MODE when ATTR_KILL_SUID is already set, causing a
> > > BUG() if the same iattr is passed to notify_change more than once.
> > >
> > > Make a copy of the struct iattr before calling notify_change.
> > >
> > > Fixes: 34b91dda7124 NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
> > > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2207969
> > > Reported-by: Zhi Li <[email protected]>
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > ---
> > > fs/nfsd/vfs.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > index c4ef24c5ffd0..ad0c5cd900b1 100644
> > > --- a/fs/nfsd/vfs.c
> > > +++ b/fs/nfsd/vfs.c
> > > @@ -538,7 +538,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > >
> > > inode_lock(inode);
> > > for (retries = 1;;) {
> > > - host_err = __nfsd_setattr(dentry, iap);
> > > + struct iattr attrs = *iap;
> > > +
> > > + host_err = __nfsd_setattr(dentry, &attrs);
> >
> > I think this needs something to ensure a well meaning by-passer doesn't
> > try to "optimise" it back to the way it was.
> > Maybe make "iap" const? Or add a comment? Or both?
> >
>
> We can't make iap const, as we have to call nfsd_sanitize_attrs on it,
> and that will change things in it. I think we'll probably have to settle
> for a comment. Chuck, can we fold the patch below in to this one?
Thanks :-)
NeilBrown
>
> --------------------8<------------------
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 2a3687cdf926..817effd63730 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -538,6 +538,11 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp,
> for (retries = 1;;) {
> struct iattr attrs;
>
> + /*
> + * notify_change can alter the iattr in ways that make it
> + * unsuitable for submission multiple times. Make a copy
> + * for every loop.
> + */
> attrs = *iap;
> host_err = __nfsd_setattr(dentry, &attrs);
> if (host_err != -EAGAIN || !retries--)
>
>