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 <bfields@xxxxxxxxxx>
>> Date: Wed Jul 15 13:31:36 2020 -0400
>>
>> nfsd4: fix NULL dereference in nfsd/clients display code
>>
>> Reported-by: NeilBrown <neilb@xxxxxxx>
>> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>>
>> 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;
>> }
>>
>> +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
>> +{
>> + struct nfsd_file *ret;
>> +
>> + spin_lock(&f->fi_lock);
>> + ret = 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, struct nfs4_stid *st)
>>
>> ds = delegstateid(st);
>> nf = st->sc_file;
>> - file = nf->fi_deleg_file;
>> + file = find_deleg_file(nf);
>> + if (!file)
>> + return 0;
>>
>> 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 <bfields@xxxxxxxxxx>
> Date: Wed Jul 15 13:31:36 2020 -0400
>
> nfsd4: fix NULL dereference in nfsd/clients display code
>
> We hold the cl_lock here, and that's enough to keep stateid's from going
> away, but it's not enough to prevent the files they point to from going
> away. Take fi_lock and a reference and check for NULL, as we do in
> other code.
>
> Reported-by: NeilBrown <neilb@xxxxxxx>
> Fixes: 78599c42ae3c ("nfsd4: add file to display list of client's opens")
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>
> 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;
> }
>
> +static struct nfsd_file *find_deleg_file(struct nfs4_file *f)
> +{
> + struct nfsd_file *ret;
> +
> + spin_lock(&f->fi_lock);
> + ret = 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;
>
> @@ -2444,6 +2454,8 @@ static int nfs4_show_open(struct seq_file *s, struct nfs4_stid *st)
> oo = ols->st_stateowner;
> nf = st->sc_file;
> file = find_any_file(nf);
> + if (!file)
> + return 0;
>
> seq_printf(s, "- ");
> nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2481,6 +2493,8 @@ static int nfs4_show_lock(struct seq_file *s, struct nfs4_stid *st)
> oo = ols->st_stateowner;
> nf = st->sc_file;
> file = find_any_file(nf);
> + if (!file)
> + return 0;
>
> seq_printf(s, "- ");
> nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2513,7 +2527,9 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
>
> ds = delegstateid(st);
> nf = st->sc_file;
> - file = nf->fi_deleg_file;
> + file = find_deleg_file(nf);
> + if (!file)
> + return 0;
>
> seq_printf(s, "- ");
> nfs4_show_stateid(s, &st->sc_stateid);
> @@ -2529,6 +2545,7 @@ static int nfs4_show_deleg(struct seq_file *s, struct nfs4_stid *st)
> seq_printf(s, ", ");
> nfs4_show_fname(s, file);
> seq_printf(s, " }\n");
> + nfsd_file_put(file);
>
> return 0;
> }
Attachment:
signature.asc
Description: PGP signature
