Message ID | 20230517162645.254512-1-jlayton@kernel.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1278418vqo; Wed, 17 May 2023 09:50:35 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ48RvV3p14JqNlV4UKOS1EAkXp8bYQQhSNYTkaM1aXGs3t1NoHq/nqDBxHmF+kP2n3z/a2K X-Received: by 2002:a05:6a00:21c2:b0:647:d698:56d2 with SMTP id t2-20020a056a0021c200b00647d69856d2mr262954pfj.27.1684342234908; Wed, 17 May 2023 09:50:34 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1684342234; cv=none; d=google.com; s=arc-20160816; b=yDd6ngAdRJqhO5Xzr/qw+1OQ4LI2TzuHL+O/2emWiDVZuG/pcLaPLyaVxfOo80qUUu gzabA/PG4ZO3jZy5tmk79j8yn/sOztO1BT6lMQV3z7a8bNAxwDv9ntcNbbCJAdc/AkAm HotDtlhQexNZUR/Tb80R5McSWMjMfxFbVPPxJRZnNm6nMm0kyO7mRUMmW6bT3kqxcGJ1 CfGlbzV/C7Ud3oeKt9ZmWlmAIIE5NC14nagPTEDwbIawotgDyCQcJBFKNyTX7n0DCImp Fy27RNrlWgZq2MZRW+IPNISvkRtNQcTZtoL9E9gORMqhfLm09hmUAai5lKcgr0b/TEN8 quwg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=V6ve4mbMhTVqlY5aA6uqD1clj3nrbbL1ZD0I7vrJbmc=; b=ujV+UaZE7nUJLWr5ZWBlVwtzcy8atNj5+fIQoagRkgmw8154ebjHHgjmKq8av9CMoj PjrWOJ0XL/DzI3FaBOHv286wJVa0aRyRK4fD/faeOu+lugC7OG2aXmAsWZQO8BnhP3Dw ImJyDTjq7F5Z3bs3Ez5P4rBkbpArwsUVr7n2cizKBZX9KCi8x/LB2ZKdOB3LRTmeIuLs du8qnmkjWI6OBsUe9/MLOWtPI46j9x3/3ijAAxrV7y+lv9Td7gekduZqnzlUGfrhXkAa QoG5T8xad47jlAZUUAf9p6XASAKYRxsWo2aqbkg2rzWy2yyzApzXOrqWduncgQJikETu bnMw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=sZJlHjmz; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i8-20020a639d08000000b0051ba2ec8320si21089133pgd.353.2023.05.17.09.50.22; Wed, 17 May 2023 09:50:34 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=sZJlHjmz; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229660AbjEQQ0v (ORCPT <rfc822;abdi.embedded@gmail.com> + 99 others); Wed, 17 May 2023 12:26:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53364 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229634AbjEQQ0s (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 17 May 2023 12:26:48 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F3137130; Wed, 17 May 2023 09:26:47 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 8FF0D63D9C; Wed, 17 May 2023 16:26:47 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 83718C433D2; Wed, 17 May 2023 16:26:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1684340807; bh=sU1C7XJg/7VvloWpwX9yx2Ma9fhNOkvoRR16fGxVrx4=; h=From:To:Cc:Subject:Date:From; b=sZJlHjmztkvkLKxHbNj52fhsNhecUqM37bGatmGQxktUqEdItN6NL5LT81V2t091h gPcnMFAc4lv9o6eMpujnLyfiItRFzKYUVq04zMGhsHGW8TlWs6aPW8/qBYsGTQd3bH Yqul0tjCKB1OEQlFvxnj6oyty7r0DvSw62ycW3nV5MfdlkgPfgzublOcRIf2hxfnup wO6fGalPgH4Yha0Fy6jCA5HXWHpPEpcQFbaO9ayWcfJ1QMlfeEWH4TpdoLo2N6cIn9 XdrYaNigaP+qblqjiHbOFziGE0A/8WhqPj7v0QnQB09sTVfVptanFsFJrBRx2ONgce J9jFpKhWTDKag== From: Jeff Layton <jlayton@kernel.org> To: Chuck Lever <chuck.lever@oracle.com> Cc: Zhi Li <yieli@redhat.com>, linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] nfsd: make a copy of struct iattr before calling notify_change Date: Wed, 17 May 2023 12:26:44 -0400 Message-Id: <20230517162645.254512-1-jlayton@kernel.org> X-Mailer: git-send-email 2.40.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766160843215060375?= X-GMAIL-MSGID: =?utf-8?q?1766160843215060375?= |
Series |
nfsd: make a copy of struct iattr before calling notify_change
|
|
Commit Message
Jeff Layton
May 17, 2023, 4:26 p.m. UTC
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 <yieli@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
fs/nfsd/vfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Comments
> On May 17, 2023, at 12:26 PM, Jeff Layton <jlayton@kernel.org> 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 <yieli@redhat.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > 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 Wed, 2023-05-17 at 17:47 +0000, Chuck Lever III wrote: > > > On May 17, 2023, at 12:26 PM, Jeff Layton <jlayton@kernel.org> 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 <yieli@redhat.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > 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. 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; ... > > + > > + 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 <jlayton@kernel.org> wrote: > > On Wed, 2023-05-17 at 17:47 +0000, Chuck Lever III wrote: >> >>> On May 17, 2023, at 12:26 PM, Jeff Layton <jlayton@kernel.org> 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 <yieli@redhat.com> >>> Signed-off-by: Jeff Layton <jlayton@kernel.org> >>> --- >>> 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 <jlayton@kernel.org> -- Chuck Lever
On Wed, 2023-05-17 at 19:13 +0000, Chuck Lever III wrote: > > > On May 17, 2023, at 3:05 PM, Jeff Layton <jlayton@kernel.org> wrote: > > > > On Wed, 2023-05-17 at 17:47 +0000, Chuck Lever III wrote: > > > > > > > On May 17, 2023, at 12:26 PM, Jeff Layton <jlayton@kernel.org> 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 <yieli@redhat.com> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > > --- > > > > 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? > Not yet. I just cooked it up this morning. I built a test kernel but testing it will take some time since it depends on load. > > > 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. > Your call. I'm fairly certain that the patch does the right thing as-is, but if you think it makes it more readable, then OK.
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 <yieli@redhat.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > 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 <yieli@redhat.com> Cheers, Jeff
> On May 19, 2023, at 6:10 AM, Jeff Layton <jlayton@kernel.org> wrote: > > 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 <yieli@redhat.com> >> Signed-off-by: Jeff Layton <jlayton@kernel.org> >> --- >> 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 <yieli@redhat.com> Thanks to you both. Applied to nfsd-fixes for the next 6.4-rc PR. -- Chuck Lever
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 <yieli@redhat.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > 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 <yieli@redhat.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > 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 <yieli@redhat.com> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > 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 <yieli@redhat.com> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > > --- > > > 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--) > >
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))