Re: [PATCH v6 05/10] NFSD first draft of async copy

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Feb 15, 2018 at 02:59:14PM -0500, Olga Kornievskaia wrote:
> On Thu, Jan 25, 2018 at 5:04 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > Nit: this could use a better subject line.
> >
> > On Tue, Oct 24, 2017 at 01:47:47PM -0400, Olga Kornievskaia wrote:
> > ...
> >> +     if (!copy->cp_synchronous) {
> >> +             status = nfsd4_init_copy_res(copy, 0);
> >> +             async_copy = kzalloc(sizeof(struct nfsd4_copy), GFP_KERNEL);
> >> +             if (!async_copy) {
> >> +                     status = nfserrno(-ENOMEM);
> >> +                     goto out;
> >> +             }
> >> +             dup_copy_fields(copy, async_copy);
> >> +             memcpy(&copy->cp_res.cb_stateid, &copy->cp_dst_stateid,
> >> +                     sizeof(copy->cp_dst_stateid));
> >> +             spin_lock(&async_copy->cp_clp->async_lock);
> >> +             list_add(&async_copy->copies,
> >> +                             &async_copy->cp_clp->async_copies);
> >> +             spin_unlock(&async_copy->cp_clp->async_lock);
> >
> > At this point other threads could in theory look up this async_copy, but
> > its copy_task field is not yet initialized.  I don't *think* that's a
> > problem for nfsd4_shutdown_copy, because I don't think the server could
> > be processing rpc's for this client any more at that point.  But I think
> > a malicious client might be able to trigger a NULL dereference in
> > nfsd4_offload_cancel.
> >
> > Is there any reason not to assign copy_task before adding it to this
> > list?
> 
> Now that I'm making changes I don't believe this is an issue. A client
> can't send nfsd4_offload_cancel() because it needs a copy stateid to
> send it with. And at this point the copy has not been replied to.

Right, but a malicious client might guess that copy stateid before it
gets the reply.

We want to make sure we're safe from crashing even on input that is very
unlikely.

--b.