Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Bruce Fields" <bfields@fieldses.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 02/19] NFSD: handle error better in write_ports_addfd()
In-reply-to: <3B93593B-861F-4816-9259-EBB20FDB4375@oracle.com>
References: <163763078330.7284.10141477742275086758.stgit@noble.brown>,
 <163763097543.7284.12067402792054742606.stgit@noble.brown>,
 <3B93593B-861F-4816-9259-EBB20FDB4375@oracle.com>
Date:   Mon, 29 Nov 2021 10:49:25 +1100
Message-id: <163814336542.26075.9044219405065931614@noble.neil.brown.name>
Precedence: bulk

On Wed, 24 Nov 2021, Chuck Lever III wrote:
>=20
> > On Nov 22, 2021, at 8:29 PM, NeilBrown <neilb@suse.de> wrote:
> >=20
> > If write_ports_add() fails, we shouldn't destroy the serv, unless we had
> > only just created it.  So if there are any permanent sockets already
> > attached, leave the serv in place.
> >=20
> > Signed-off-by: NeilBrown <neilb@suse.de>
>=20
> This needs to go at the front of the series, IMO, to make it
> more straightforward to backport if needed.

That's reasonable.

>=20
> Though ea068bad27ce ("NFSD: move lockd_up() before svc_addsock()")
> appears to have introduced "if (err < 0)" I'm not sure that's
> actually where problems were introduced. Is Cc: stable warranted?

I don't think Cc: stable is warranted.  I think far too much goes to
'stable', but also not enough....  So I'm selective.

The problem fixed is barely a bug - just a minor inconvenience.
In practice, svc_addsock() doesn't fail, because it is never asked to do
something that it cannot do.  So handling failure graceful will only be
noticed by someone who is doing strange things.
So while we should definitely fix it, I'm not inclined to backport the
fix.=20

BTW, I think the "bug" was introduced in Commit 0cd14a061e32 ("nfsd: fix
error handling in __write_ports_addxprt"), which fixed a different
(real) bug introduced by the patch you identified.

Thanks,
NeilBrown

>=20
>=20
> > ---
> > fs/nfsd/nfsctl.c |    2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >=20
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index 5eb564e58a9b..93d417871302 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -742,7 +742,7 @@ static ssize_t __write_ports_addfd(char *buf, struct =
net *net, const struct cred
> > 		return err;
> >=20
> > 	err =3D svc_addsock(nn->nfsd_serv, fd, buf, SIMPLE_TRANSACTION_LIMIT, cr=
ed);
> > -	if (err < 0) {
> > +	if (err < 0 && list_empty(&nn->nfsd_serv->sv_permsocks)) {
> > 		nfsd_put(net);
> > 		return err;
> > 	}
> >=20
> >=20
>=20
> --
> Chuck Lever
>=20
>=20
>=20
>=20