handle_async_copy calling kzalloc under spinlock

handle_async_copy calling kzalloc under spinlock

[J. Bruce Fields]

From bc0c9079b48d "NFS handle COPY reply CB_OFFLOAD call race":

+       spin_lock(&server->nfs_client->cl_lock);
+       list_for_each_entry(copy, &server->nfs_client->pending_cb_stateids,
+                               copies) {
+               if (memcmp(&res->write_res.stateid, &copy->stateid,
+                               NFS4_STATEID_SIZE))
+                       continue;
+               found_pending = true;
+               list_del(&copy->copies);
+               break;
+       }
+       if (found_pending) {
+               spin_unlock(&server->nfs_client->cl_lock);
+               goto out;
+       }
 
        copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);

At this point we're still holding cl_lock.

Best might be to allocate "copy" before taking the lock, then free it on any
paths where we don't end up needing it.

--b.

Re: handle_async_copy calling kzalloc under spinlock

[J. Bruce Fields]

On Fri, Nov 16, 2018 at 11:25:50AM -0500, Olga Kornievskaia wrote:
> On Fri, Nov 16, 2018 at 9:27 AM J. Bruce Fields <bfields@fieldses.org>
> wrote:
> 
> > From bc0c9079b48d "NFS handle COPY reply CB_OFFLOAD call race":
> >
> > +       spin_lock(&server->nfs_client->cl_lock);
> > +       list_for_each_entry(copy, &server->nfs_client->pending_cb_stateids,
> > +                               copies) {
> > +               if (memcmp(&res->write_res.stateid, &copy->stateid,
> > +                               NFS4_STATEID_SIZE))
> > +                       continue;
> > +               found_pending = true;
> > +               list_del(&copy->copies);
> > +               break;
> > +       }
> > +       if (found_pending) {
> > +               spin_unlock(&server->nfs_client->cl_lock);
> > +               goto out;
> > +       }
> >
> >         copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
> >
> > At this point we're still holding cl_lock.
> >
> > Best might be to allocate "copy" before taking the lock, then free it on
> > any
> > paths where we don't end up needing it.
> >
> >
> Thanks. I'll do that.

Thanks.  And, I just noticed--nfs4_callback_offload has the same
problem.

--b.

Re: handle_async_copy calling kzalloc under spinlock

[J. Bruce Fields]

On Fri, Nov 16, 2018 at 12:56:45PM -0500, J. Bruce Fields wrote:
> On Fri, Nov 16, 2018 at 11:25:50AM -0500, Olga Kornievskaia wrote:
> > On Fri, Nov 16, 2018 at 9:27 AM J. Bruce Fields <bfields@fieldses.org>
> > wrote:
> > 
> > > From bc0c9079b48d "NFS handle COPY reply CB_OFFLOAD call race":
> > >
> > > +       spin_lock(&server->nfs_client->cl_lock);
> > > +       list_for_each_entry(copy, &server->nfs_client->pending_cb_stateids,
> > > +                               copies) {
> > > +               if (memcmp(&res->write_res.stateid, &copy->stateid,
> > > +                               NFS4_STATEID_SIZE))
> > > +                       continue;
> > > +               found_pending = true;
> > > +               list_del(&copy->copies);
> > > +               break;
> > > +       }
> > > +       if (found_pending) {
> > > +               spin_unlock(&server->nfs_client->cl_lock);
> > > +               goto out;
> > > +       }
> > >
> > >         copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
> > >
> > > At this point we're still holding cl_lock.
> > >
> > > Best might be to allocate "copy" before taking the lock, then free it on
> > > any
> > > paths where we don't end up needing it.
> > >
> > >
> > Thanks. I'll do that.
> 
> Thanks.  And, I just noticed--nfs4_callback_offload has the same
> problem.

By the way, I don't understand the create case in that code--if you get
a CB_OFFLOAD without already having a matching copy stateid, shouldn't
you just return an error and forget about it?

I also wonder if SERVERFAULT is really the best error for a memory
allocation failure there.

--b.

Re: handle_async_copy calling kzalloc under spinlock

[Olga Kornievskaia]

On Fri, Nov 16, 2018 at 1:01 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Nov 16, 2018 at 12:56:45PM -0500, J. Bruce Fields wrote:
> > On Fri, Nov 16, 2018 at 11:25:50AM -0500, Olga Kornievskaia wrote:
> > > On Fri, Nov 16, 2018 at 9:27 AM J. Bruce Fields <bfields@fieldses.org>
> > > wrote:
> > >
> > > > From bc0c9079b48d "NFS handle COPY reply CB_OFFLOAD call race":
> > > >
> > > > +       spin_lock(&server->nfs_client->cl_lock);
> > > > +       list_for_each_entry(copy, &server->nfs_client->pending_cb_stateids,
> > > > +                               copies) {
> > > > +               if (memcmp(&res->write_res.stateid, &copy->stateid,
> > > > +                               NFS4_STATEID_SIZE))
> > > > +                       continue;
> > > > +               found_pending = true;
> > > > +               list_del(&copy->copies);
> > > > +               break;
> > > > +       }
> > > > +       if (found_pending) {
> > > > +               spin_unlock(&server->nfs_client->cl_lock);
> > > > +               goto out;
> > > > +       }
> > > >
> > > >         copy = kzalloc(sizeof(struct nfs4_copy_state), GFP_NOFS);
> > > >
> > > > At this point we're still holding cl_lock.
> > > >
> > > > Best might be to allocate "copy" before taking the lock, then free it on
> > > > any
> > > > paths where we don't end up needing it.
> > > >
> > > >
> > > Thanks. I'll do that.
> >
> > Thanks.  And, I just noticed--nfs4_callback_offload has the same
> > problem.

nfs4_callback_offload is where I changed it. I see now,
handle_async_copy() in nfs42proc.c also has it.


> By the way, I don't understand the create case in that code--if you get
> a CB_OFFLOAD without already having a matching copy stateid, shouldn't
> you just return an error and forget about it?

Then how does the copy knows not to go wait for the callback? Copy
checks the pending_callback list to see if received a callback. If
not, it puts itself on the copy list and goes to sleep. The callback,
checks the copy list and if it finds a copy signals it, if not it puts
itself on the pending_callback list. a lock is held over checking one
list and putting yourself on the other.

> I also wonder if SERVERFAULT is really the best error for a memory
> allocation failure there.

I guess EIO or ENOMEM might be better. But I don't think this error
gets returned anywhere to the main process.
>
> --b.

Re: handle_async_copy calling kzalloc under spinlock

[J. Bruce Fields]

On Fri, Nov 16, 2018 at 01:52:29PM -0500, Olga Kornievskaia wrote:
> On Fri, Nov 16, 2018 at 1:30 PM Olga Kornievskaia <aglo@umich.edu> wrote:
> > Then how does the copy knows not to go wait for the callback? Copy
> > checks the pending_callback list to see if received a callback. If
> > not, it puts itself on the copy list and goes to sleep. The callback,
> > checks the copy list and if it finds a copy signals it, if not it puts
> > itself on the pending_callback list. a lock is held over checking one
> > list and putting yourself on the other.

OK, apologies, I don't really understand those data structures yet, but
something seems wrong to me.

Under what circumstances could we recieve a CB_OFFLOAD without having
started the corresponding copy already?

And shouldn't CB_OFFLOAD be returning bad_stateid in the case it doesn't
recognize the given stateid?  It looks like the allocation failure is
the *only* way we'll return an error on CB_OFFLOAD, and that seems
wrong.

> > > I also wonder if SERVERFAULT is really the best error for a memory
> > > allocation failure there.
> >
> > I guess EIO or ENOMEM might be better. But I don't think this error
> > gets returned anywhere to the main process.
> >
> 
> Wait. It is returning SERVERFAULT because it's the callback server replying
> back to the server's CB_RECALL call and I believe SERVERFAULT is the
> appropriate error here. NFS doesn't have ENOMEM error.

We could return DELAY if we think it might be worth the server trying
the CB_RECALL again.  (That's what nfsd usually returns on allocation
failures.  I don't know if that's really ideal.)

--b.

Re: handle_async_copy calling kzalloc under spinlock

[Olga Kornievskaia]

On Fri, Nov 16, 2018 at 2:30 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Nov 16, 2018 at 01:52:29PM -0500, Olga Kornievskaia wrote:
> > On Fri, Nov 16, 2018 at 1:30 PM Olga Kornievskaia <aglo@umich.edu> wrote:
> > > Then how does the copy knows not to go wait for the callback? Copy
> > > checks the pending_callback list to see if received a callback. If
> > > not, it puts itself on the copy list and goes to sleep. The callback,
> > > checks the copy list and if it finds a copy signals it, if not it puts
> > > itself on the pending_callback list. a lock is held over checking one
> > > list and putting yourself on the other.
>
> OK, apologies, I don't really understand those data structures yet, but
> something seems wrong to me.
>
> Under what circumstances could we recieve a CB_OFFLOAD without having
> started the corresponding copy already?

It can receive a CB_OFFLOAD before it receives a reply to the COPY.
It's possible and I can trigger it during testing when doing a really
short copy. The copy is done and callback thread sends a reply.
CB_OFFLOAD call and COPY reply can be switched on the server or on the
processing on the client.

>
> And shouldn't CB_OFFLOAD be returning bad_stateid in the case it doesn't
> recognize the given stateid?

It could but what should the server do in this case. I would imagine
it wouldn't do anything. There is nothing it can do. So now we have a
copy that send the call and is going to wait on the reply which will
never come as the 1st one came and we rejected it and now copy will
wait forever.

Please describe what "is wrong" with the current implementation.  I
believe it provide a reasonable solution to the race condition.

> It looks like the allocation failure is
> the *only* way we'll return an error on CB_OFFLOAD, and that seems
> wrong.

Yes it is the only error we currently return. Do you see any other
useful errors that a client should return (and would be useful to
handle on the server). I don't see any need for any more
complications.

> > > > I also wonder if SERVERFAULT is really the best error for a memory
> > > > allocation failure there.
> > >
> > > I guess EIO or ENOMEM might be better. But I don't think this error
> > > gets returned anywhere to the main process.
> > >
> >
> > Wait. It is returning SERVERFAULT because it's the callback server replying
> > back to the server's CB_RECALL call and I believe SERVERFAULT is the
> > appropriate error here. NFS doesn't have ENOMEM error.
>
> We could return DELAY if we think it might be worth the server trying
> the CB_RECALL again.  (That's what nfsd usually returns on allocation
> failures.  I don't know if that's really ideal.)

If the client had any smarts to say correct this error that would be
useful to return but this is not the case. I don't believe there is a

Re: handle_async_copy calling kzalloc under spinlock

[J. Bruce Fields]

On Fri, Nov 16, 2018 at 02:49:00PM -0500, Olga Kornievskaia wrote:
> On Fri, Nov 16, 2018 at 2:30 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> >
> > On Fri, Nov 16, 2018 at 01:52:29PM -0500, Olga Kornievskaia wrote:
> > > On Fri, Nov 16, 2018 at 1:30 PM Olga Kornievskaia <aglo@umich.edu> wrote:
> > > > Then how does the copy knows not to go wait for the callback? Copy
> > > > checks the pending_callback list to see if received a callback. If
> > > > not, it puts itself on the copy list and goes to sleep. The callback,
> > > > checks the copy list and if it finds a copy signals it, if not it puts
> > > > itself on the pending_callback list. a lock is held over checking one
> > > > list and putting yourself on the other.
> >
> > OK, apologies, I don't really understand those data structures yet, but
> > something seems wrong to me.
> >
> > Under what circumstances could we recieve a CB_OFFLOAD without having
> > started the corresponding copy already?
> 
> It can receive a CB_OFFLOAD before it receives a reply to the COPY.
> It's possible and I can trigger it during testing when doing a really
> short copy. The copy is done and callback thread sends a reply.
> CB_OFFLOAD call and COPY reply can be switched on the server or on the
> processing on the client.

That race is discussed in
https://tools.ietf.org/html/rfc5661#section-2.10.6.3 and is supposed to
be dealt with by using referring triples and/or returning DELAY.

> > And shouldn't CB_OFFLOAD be returning bad_stateid in the case it doesn't
> > recognize the given stateid?
> 
> It could but what should the server do in this case. I would imagine
> it wouldn't do anything. There is nothing it can do. So now we have a
> copy that send the call and is going to wait on the reply which will
> never come as the 1st one came and we rejected it and now copy will
> wait forever.
> 
> Please describe what "is wrong" with the current implementation.  I
> believe it provide a reasonable solution to the race condition.

Looks like a server that sends bad stateids in callbacks could cause you
to allocate something that will never get freed.

--b.

> > It looks like the allocation failure is
> > the *only* way we'll return an error on CB_OFFLOAD, and that seems
> > wrong.
> 
> Yes it is the only error we currently return. Do you see any other
> useful errors that a client should return (and would be useful to
> handle on the server). I don't see any need for any more
> complications.
> 
> > > > > I also wonder if SERVERFAULT is really the best error for a memory
> > > > > allocation failure there.
> > > >
> > > > I guess EIO or ENOMEM might be better. But I don't think this error
> > > > gets returned anywhere to the main process.
> > > >
> > >
> > > Wait. It is returning SERVERFAULT because it's the callback server replying
> > > back to the server's CB_RECALL call and I believe SERVERFAULT is the
> > > appropriate error here. NFS doesn't have ENOMEM error.
> >
> > We could return DELAY if we think it might be worth the server trying
> > the CB_RECALL again.  (That's what nfsd usually returns on allocation
> > failures.  I don't know if that's really ideal.)
> 
> If the client had any smarts to say correct this error that would be
> useful to return but this is not the case. I don't believe there is a

Re: handle_async_copy calling kzalloc under spinlock

[Olga Kornievskaia]

On Fri, Nov 16, 2018 at 2:58 PM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Fri, Nov 16, 2018 at 02:49:00PM -0500, Olga Kornievskaia wrote:
> > On Fri, Nov 16, 2018 at 2:30 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > >
> > > On Fri, Nov 16, 2018 at 01:52:29PM -0500, Olga Kornievskaia wrote:
> > > > On Fri, Nov 16, 2018 at 1:30 PM Olga Kornievskaia <aglo@umich.edu> wrote:
> > > > > Then how does the copy knows not to go wait for the callback? Copy
> > > > > checks the pending_callback list to see if received a callback. If
> > > > > not, it puts itself on the copy list and goes to sleep. The callback,
> > > > > checks the copy list and if it finds a copy signals it, if not it puts
> > > > > itself on the pending_callback list. a lock is held over checking one
> > > > > list and putting yourself on the other.
> > >
> > > OK, apologies, I don't really understand those data structures yet, but
> > > something seems wrong to me.
> > >
> > > Under what circumstances could we recieve a CB_OFFLOAD without having
> > > started the corresponding copy already?
> >
> > It can receive a CB_OFFLOAD before it receives a reply to the COPY.
> > It's possible and I can trigger it during testing when doing a really
> > short copy. The copy is done and callback thread sends a reply.
> > CB_OFFLOAD call and COPY reply can be switched on the server or on the
> > processing on the client.
>
> That race is discussed in
> https://tools.ietf.org/html/rfc5661#section-2.10.6.3 and is supposed to
> be dealt with by using referring triples and/or returning DELAY.

I believe those are suggestions and not mandates? A client can't rely
that the server will implement referring sequence information. Sending
"delay" to the server might be an option but it's an option that most
like will interfere with performance as well?

> > > And shouldn't CB_OFFLOAD be returning bad_stateid in the case it doesn't
> > > recognize the given stateid?
> >
> > It could but what should the server do in this case. I would imagine
> > it wouldn't do anything. There is nothing it can do. So now we have a
> > copy that send the call and is going to wait on the reply which will
> > never come as the 1st one came and we rejected it and now copy will
> > wait forever.
> >
> > Please describe what "is wrong" with the current implementation.  I
> > believe it provide a reasonable solution to the race condition.
>
> Looks like a server that sends bad stateids in callbacks could cause you
> to allocate something that will never get freed.

I thought the philosophy was that client shouldn't be coded to a
broken server. If needed, we can later on add a cleanup thread that
goes thru the list and removes really old entries.

>
> --b.
>
> > > It looks like the allocation failure is
> > > the *only* way we'll return an error on CB_OFFLOAD, and that seems
> > > wrong.
> >
> > Yes it is the only error we currently return. Do you see any other
> > useful errors that a client should return (and would be useful to
> > handle on the server). I don't see any need for any more
> > complications.
> >
> > > > > > I also wonder if SERVERFAULT is really the best error for a memory
> > > > > > allocation failure there.
> > > > >
> > > > > I guess EIO or ENOMEM might be better. But I don't think this error
> > > > > gets returned anywhere to the main process.
> > > > >
> > > >
> > > > Wait. It is returning SERVERFAULT because it's the callback server replying
> > > > back to the server's CB_RECALL call and I believe SERVERFAULT is the
> > > > appropriate error here. NFS doesn't have ENOMEM error.
> > >
> > > We could return DELAY if we think it might be worth the server trying
> > > the CB_RECALL again.  (That's what nfsd usually returns on allocation
> > > failures.  I don't know if that's really ideal.)
> >
> > If the client had any smarts to say correct this error that would be
> > useful to return but this is not the case. I don't believe there is a

Re: handle_async_copy calling kzalloc under spinlock

[J. Bruce Fields]

On Fri, Nov 16, 2018 at 03:11:58PM -0500, Olga Kornievskaia wrote:
> On Fri, Nov 16, 2018 at 2:58 PM J. Bruce Fields <bfields@fieldses.org> wrote:
> > That race is discussed in
> > https://tools.ietf.org/html/rfc5661#section-2.10.6.3 and is supposed to
> > be dealt with by using referring triples and/or returning DELAY.
> 
> I believe those are suggestions and not mandates? A client can't rely
> that the server will implement referring sequence information. Sending
> "delay" to the server might be an option but it's an option that most
> like will interfere with performance as well?

Yes, I suppose the server either needs to implement referring triples or
retry pretty aggressively.

(By the way, I wonder if the server should always do synchronous copies
for copies smaller than a certain threshhold.  Might be hard to choose
the threshhold, though.)

> > > > And shouldn't CB_OFFLOAD be returning bad_stateid in the case it doesn't
> > > > recognize the given stateid?
> > >
> > > It could but what should the server do in this case. I would imagine
> > > it wouldn't do anything. There is nothing it can do. So now we have a
> > > copy that send the call and is going to wait on the reply which will
> > > never come as the 1st one came and we rejected it and now copy will
> > > wait forever.
> > >
> > > Please describe what "is wrong" with the current implementation.  I
> > > believe it provide a reasonable solution to the race condition.
> >
> > Looks like a server that sends bad stateids in callbacks could cause you
> > to allocate something that will never get freed.
> 
> I thought the philosophy was that client shouldn't be coded to a
> broken server. If needed, we can later on add a cleanup thread that
> goes thru the list and removes really old entries.

I suppose so.

I don't know, this design still makes me pretty uncomfortable, but I
guess I haven't come up with a strong reason it couldn't work.

--b.