From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.0 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 63A15C433E4 for ; Thu, 16 Jul 2020 23:43:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4BB8C2063A for ; Thu, 16 Jul 2020 23:43:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726166AbgGPXnr (ORCPT ); Thu, 16 Jul 2020 19:43:47 -0400 Received: from mx2.suse.de ([195.135.220.15]:51462 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726125AbgGPXnr (ORCPT ); Thu, 16 Jul 2020 19:43:47 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 56D08AB89; Thu, 16 Jul 2020 23:43:49 +0000 (UTC) From: NeilBrown To: "J. Bruce Fields" Date: Fri, 17 Jul 2020 09:43:40 +1000 Cc: Jeff Layton , linux-nfs@vger.kernel.org Subject: Re: nfs4_show_superblock considered harmful :-) In-Reply-To: <20200716171958.GB18568@fieldses.org> References: <871rn38suc.fsf@notabene.neil.brown.name> <20200529220608.GA22758@fieldses.org> <87a71n7dek.fsf@notabene.neil.brown.name> <20200715185456.GE15543@fieldses.org> <20200716171958.GB18568@fieldses.org> Message-ID: <87tuy7vxeb.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On Thu, Jul 16 2020, J. Bruce Fields wrote: > On Wed, Jul 15, 2020 at 02:54:56PM -0400, J. Bruce Fields wrote: >> commit 4eef57aa4fc0 >> Author: J. Bruce Fields >> Date: Wed Jul 15 13:31:36 2020 -0400 >>=20 >> nfsd4: fix NULL dereference in nfsd/clients display code >>=20=20=20=20=20 >> Reported-by: NeilBrown >> Signed-off-by: J. Bruce Fields >>=20 >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index ab5c8857ae5a..08b8376c74d7 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f) >> return ret; >> } >>=20=20 >> +static struct nfsd_file *find_deleg_file(struct nfs4_file *f) >> +{ >> + struct nfsd_file *ret; >> + >> + spin_lock(&f->fi_lock); >> + ret =3D nfsd_file_get(f->fi_deleg_file); >> + spin_unlock(&f->fi_lock); >> + return ret; >> +} >> + > ... >> @@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, str= uct nfs4_stid *st) >>=20=20 >> ds =3D delegstateid(st); >> nf =3D st->sc_file; >> - file =3D nf->fi_deleg_file; >> + file =3D find_deleg_file(nf); >> + if (!file) >> + return 0; >>=20=20 >> seq_printf(s, "- "); >> nfs4_show_stateid(s, &st->sc_stateid); > > Oops, I added a "get" without a corresponding "put". > > --b. > > commit 8d2edfe45286 > Author: J. Bruce Fields > Date: Wed Jul 15 13:31:36 2020 -0400 > > nfsd4: fix NULL dereference in nfsd/clients display code >=20=20=20=20=20 > We hold the cl_lock here, and that's enough to keep stateid's from go= ing > away, but it's not enough to prevent the files they point to from goi= ng > away. Take fi_lock and a reference and check for NULL, as we do in > other code. >=20=20=20=20=20 > Reported-by: NeilBrown > Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's ope= ns") > Signed-off-by: J. Bruce Fields > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index ab5c8857ae5a..9d45117c8c18 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -507,6 +507,16 @@ find_any_file(struct nfs4_file *f) > return ret; > } >=20=20 > +static struct nfsd_file *find_deleg_file(struct nfs4_file *f) > +{ > + struct nfsd_file *ret; > + > + spin_lock(&f->fi_lock); > + ret =3D nfsd_file_get(f->fi_deleg_file); A test on f->fi_deleg_file being non-NULL would make this look safer. It would also make the subsequent test on the return value appear sane. Thanks, NeilBrown > + spin_unlock(&f->fi_lock); > + return ret; > +} > + > static atomic_long_t num_delegations; > unsigned long max_delegations; >=20=20 > @@ -2444,6 +2454,8 @@ static int nfs4_show_open(struct seq_file *s, struc= t nfs4_stid *st) > oo =3D ols->st_stateowner; > nf =3D st->sc_file; > file =3D find_any_file(nf); > + if (!file) > + return 0; >=20=20 > seq_printf(s, "- "); > nfs4_show_stateid(s, &st->sc_stateid); > @@ -2481,6 +2493,8 @@ static int nfs4_show_lock(struct seq_file *s, struc= t nfs4_stid *st) > oo =3D ols->st_stateowner; > nf =3D st->sc_file; > file =3D find_any_file(nf); > + if (!file) > + return 0; >=20=20 > seq_printf(s, "- "); > nfs4_show_stateid(s, &st->sc_stateid); > @@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, stru= ct nfs4_stid *st) >=20=20 > ds =3D delegstateid(st); > nf =3D st->sc_file; > - file =3D nf->fi_deleg_file; > + file =3D find_deleg_file(nf); > + if (!file) > + return 0; >=20=20 > seq_printf(s, "- "); > nfs4_show_stateid(s, &st->sc_stateid); > @@ -2529,6 +2545,7 @@ static int nfs4_show_deleg(struct seq_file *s, stru= ct nfs4_stid *st) > seq_printf(s, ", "); > nfs4_show_fname(s, file); > seq_printf(s, " }\n"); > + nfsd_file_put(file); >=20=20 > return 0; > } --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAl8Q5iwACgkQOeye3VZi gbkgVw/7B6AED/Dyay+AmC5fSq9d1f65zxJWQkFAn1wQ6fTXGIMhGFgqWH2JSY9y 7v38meJfAUcvhjclBNX3nngJaNLeEBFo4YlmMJkuHR4aIFDAIN6BabuJX2dUrvsW D4PBPX6mfPLywkMuWFoxGANkP8BGXUMHN7gU/9+rvHdKEupnocSEiay9+dyBvC3H le0JDW63hHiWHsEWWu1LnQ28JfaVoVUhy7gv4n7/eHiRaP4rZTbS+ArCskAeEC5N fJ1ZT+h3AxiyPlqiliNLSkj3XBKw3XYlyQFpx3FIEoqVFZ3Nav6MEsu/4eFbmkRm jgoGC3DtFOAS/OMOdBzSzsEmXLht+D+KUBZLS66MBHkrBqzLcpkxKgI32NPQUu/n UyIetlGfHoxM0w8S4hRQqSf/y9qg8n7lML/bHwqJW7E0wKWov1OTy0YL9B929duS HS5zmuWSNWTsZyyjY9X5HajyK2AsObACBdxTV59Rtc4Hokt8oaCVdPIdKnBAkKBL +1nfB9a6u7ZaV/yxvbJKkodcwhRxCLt63TT0B8CWpbUuk3oiOCkI0gfhm8WXaQhR FuewuTNHDwXFDDBjYsdFM0jdkHEd7BBfoCbQwodv9qFgzst0xUC4Y3hgRCFJsZRv urHnXluv3cXPPmv+tUVQ1ayaZG8Q/YQ3OIrArbXoPwjracBtdRI= =jfBQ -----END PGP SIGNATURE----- --=-=-=--