Re: list_del corruption / unhash_ol_stateid()

[Jeff Layton <jlayton@poochiereds.net>]

On Tue, 04 Aug 2015 16:18:10 -0400
Andrew W Elble <aweits@rit.edu> wrote:

> 
> > In any case, I think this explains where the "no readable file" warning
> > is coming from, but I'm not sure yet about the mem corruption...
> 
> Forgive my shorthand, but I think this is what we're seeing:
> 
> open2              close
>  create 1 (idr)
>  init 2 (hashed)
> 

I'm not sure I'm following your shorthand correctly, but, I'm guessing
the above is that you're creating a stateid and then calling
init_open_stateid on it to hash it?

If you can flesh out the description of the race, it might be more
clear.

>                    close preprocess_seqid 3 (local ref in nfsd4_close)
> 		   close_open_stateid 2 -> unhashed (unhashed)
> 

Ok, so here we have the client sending a simultaneous close for the
same stateid? How did it get it in the first place? We won't have sent
the reply yet to the OPEN that this CLOSE is racing with.

Unless...the initial task that created the stateid got _really_
stalled, and another OPEN raced in, found that stateid and was able to
reply quickly. Then the client could send a CLOSE for that second OPEN
before the first opener ever finished.

In any case, we really do need to make subsequent openers of a nfs4_file
wait until it's fully constructed before using it.

>  release_open_stateid 1 -> list_del corruption (because unhashed already
>                         -> should still be refcount 2?)
> 

...regardless, I do agree that the release_open_stateid call here is
probably making some assumptions that it shouldn't, and that is probably
where the list corruption is coming in. 

If you had multiple racing OPENs then one could find the stateid that
was previously hashed. A simple fix might be to just use list_del_init
calls in unhash_ol_stateid. That should make any second list_del's a
no-op.

Whether that's sufficient to fix the mem corruption, I'm not sure...

>                    nfs4_put_stid 0 -> destroyed
> 
>  nfs4_put_stid 0 -> use after free
> 
> This also explains the '6a' as the first byte, as the final
> nfs4_put_stid will decrement sc_count first. There are other permutations.
> 
> Also, the return-with-status from nfs_get_vfs_file() appears to be break_lease()
> (much further down) returning -EWOULDBLOCK (in both cases, memory
> corruption and the simple warning case)
> 
> Thanks,
> 
> Andy


-- 
Jeff Layton <jlayton@poochiereds.net>