Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1512644

Re: [PATCH v3] NFSv4: replace seqcount_t with a rw_semaphore

Show all headers | View raw

On 2016-10-31 15:30:02 [+0000], Trond Myklebust wrote:
> > On Oct 31, 2016, at 09:19, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > The list_for_each_entry() in nfs4_reclaim_open_state:
> > It seems that this lock protects the ->so_states list among other
> > atomic_t & flags members. So at the begin of the loop we inc ->count
> > ensuring that this field is not removed while we use it. So we drop the
> > ->so_lock loc during the loop it seems. And after nfs4_reclaim_locks()
> > invocation we nfs4_put_open_state() and grab the ->so_lock again. So if
> > we were the last user of this struct and we remove it, then the
> > following list_next_entry() invocation is a use-after-free. Even if we
> > use list_for_each_entry_safe() there is no guarantee that the following
> > member is still valid because it might have been removed by someone
> > invoking nfs4_put_open_state() on it, right?
> > So there is this.
> > 
> > However to address my initial problem I have here a patch :) So it uses
> > a rw_semaphore which ensures that there is only one writer at a time or
> > multiple reader. So it should be basically what is happening now plus a
> > tiny tiny tiny lock plus lockdep coverage. I tried to this myself but I
> > don't manage to get into this code path at all so I might be doing
> > something wrong.
> > 
> > Could you please check if this patch is working for you and whether my
> > list_for_each_entry() observation is correct or not?
> > 
> > v2…v3: replace the seqlock with a RW semaphore.
> > 
> 
> NACK. That will deadlock. The reason why we use a seqlock there is precisely because we cannot allow ordinary RPC calls to lock out the recovery thread. 
Hmmm. So this is getting invoked if I reboot the server? A restart of
nfs-kernel-server is the same thing?
Is the list_for_each_entry() observation I made correct?

>If the server reboots, then ordinary RPC calls will fail until the recovery thread has had a chance to re-establish the state.

This means that the ordinary RPC call won't return and fail but wait
with the lock held until the recovery thread did its thing?

Sebastian

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2] NFSv4: replace seqcount_t with a seqlock_t Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-10-28 23:40 +0200
  Re: [PATCH v2] NFSv4: replace seqcount_t with a seqlock_t Trond Myklebust <trondmy@primarydata.com> - 2016-10-29 00:30 +0200
    Re: [PATCH v2] NFSv4: replace seqcount_t with a seqlock_t Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-10-31 11:50 +0100
    [PATCH v3] NFSv4: replace seqcount_t with a rw_semaphore Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-10-31 14:30 +0100
      Re: [PATCH v3] NFSv4: replace seqcount_t with a rw_semaphore Trond Myklebust <trondmy@primarydata.com> - 2016-10-31 16:40 +0100
        Re: [PATCH v3] NFSv4: replace seqcount_t with a rw_semaphore Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-10-31 17:00 +0100
          Re: [PATCH v3] NFSv4: replace seqcount_t with a rw_semaphore Trond Myklebust <trondmy@primarydata.com> - 2016-10-31 17:20 +0100
            [PATCH v4] NFSv4: replace seqcount_t with a seqlock_t Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-02 18:20 +0100
            Re: [PATCH v3] NFSv4: replace seqcount_t with a rw_semaphore Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-11-02 18:20 +0100
              Re: [PATCH v3] NFSv4: replace seqcount_t with a rw_semaphore Trond Myklebust <trondmy@primarydata.com> - 2016-11-02 18:40 +0100

csiph-web