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

Groups > linux.kernel > #1612671 > unrolled thread

Re: deadlock in synchronize_srcu() in debugfs?

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: deadlock in synchronize_srcu() in debugfs? Nicolai Stange <nicstange@gmail.com> - 2017-03-30 09:40 +0200
    Re: deadlock in synchronize_srcu() in debugfs? Johannes Berg <johannes@sipsolutions.net> - 2017-03-30 10:00 +0200
      Re: deadlock in synchronize_srcu() in debugfs? Nicolai Stange <nicstange@gmail.com> - 2017-03-30 12:30 +0200
        Re: deadlock in synchronize_srcu() in debugfs? Johannes Berg <johannes@sipsolutions.net> - 2017-03-30 13:20 +0200
          Re: deadlock in synchronize_srcu() in debugfs? Nicolai Stange <nicstange@gmail.com> - 2017-03-31 11:10 +0200
            Re: deadlock in synchronize_srcu() in debugfs? Johannes Berg <johannes@sipsolutions.net> - 2017-03-31 11:50 +0200

#1612671 — Re: deadlock in synchronize_srcu() in debugfs?

Hi Johannes,

On Mon, Mar 27 2017, Johannes Berg wrote:

>> > Before I go hunting - has anyone seen a deadlock in
>> > synchronize_srcu() in debugfs_remove() before?
>> 
>> Not yet. How reproducible is this?
>
> So ... this turned out to be a livelock of sorts.
>
> We have a debugfs file (not upstream (yet?), it seems) that basically
> blocks reading data.
>
> At the point of system hanging, there was a process reading from that
> file, with no data being generated.

>
> A second process was trying to remove a completely unrelated debugfs
> file (*), with the RTNL held.

I wonder if holding the RTNL during the debugfs file removal is really
needed. I'll try to have a look in the next couple of days.

>
> A third and many other processes were waiting to acquire the RTNL.
>
>
> Obviously, in light of things like nfp_net_debugfs_tx_q_read(),
> wil_write_file_reset(), lowpan_short_addr_get() and quite a few more,
> nobody in the whole system can now remove debugfs files while holding
> the RTNL. Not sure how many people that affects, but it's IMHO a pretty
> major new restriction, and one that isn't even flagged at all.

To be honest, I didn't have this scenario, i.e. removing a debugfs file
under a lock, in mind when writing this removal protection series.

Thank you very much for your debugging work and for pointing me to this
sort of problem!

Summarizing, the problem is the call to the indefinitely blocking
srcu_synchronize() while having a lock held? I'll see whether I can ask
lockdep if any lock is held and spit out a warning then.


> Similarly, nobody should be blocking in debugfs files, like we did in
> ours, but also smsdvb_stats_read(), crtc_crc_open() look like they
> could block for quite a while.

Blocking in the debugfs files' fops shall be fine by itself, that's why
SRCU is used for the removal stuff.


> Again, there's no warning here that blocking in debugfs files can now
> indefinitely defer completely unrelated debugfs_remove() calls in the
> entire system.

Yes, there's only one global srcu_struct for debugfs. So far this hasn't
been a problem and if I understand things correctly, it's also not the
problem at hand? If it really becomes an issue, we can very well
introduce per directory srcu_structs as you suggested.


> Overall, while I can solve this problem for our driver, possibly by
> making the debugfs file return some dummy data periodically if no real
> data exists, which may not easily be possible for all such files, I'm
> not convinced that all of this really is the right thing to actually
> impose.

No, I agree: imposing dummy data reads certainly isn't.

Let me have a look
- whether holding the RTNL lock while removing the debugfs files is
  actually needed and
- whether there is an easy way to spot similar scenarios and emit
  a warning for them.

If this doesn't solve the problem, I'll have to think of a different way
to fix this...

> (*) before removing first first we'd obviously wake up and thereby more
> or less terminate the readers first

With the current implementation, I can't see an easy way to identify the
tasks blocking on a particular debugfs file. But maybe this is
resolvable and the way to go here...


Thanks,

Nicolai

[toc] | [next] | [standalone]

#1612685

On Thu, 2017-03-30 at 09:32 +0200, Nicolai Stange wrote:
> 
> I wonder if holding the RTNL during the debugfs file removal is
> really needed. I'll try to have a look in the next couple of days.

Yes, I'm pretty much convinced that it is. I considered doing a
deferred debugfs_remove() by holding the object around, but then I
can't be sure that I can later re-add a new object with the same
directory name, so I have much more complexity - I'm not even sure that
can be solved at all, *perhaps* by renaming in debugfs first, but
that's major new complexity.

Enough complexity that I'm considering just removing debugfs usage
entirely and invent new mechanisms, or use sysfs, or something else
instead.

> Summarizing, the problem is the call to the indefinitely blocking
> srcu_synchronize() while having a lock held? I'll see whether I can
> ask lockdep if any lock is held and spit out a warning then.

Half the thread here was about that - it's not easily doable because
you'd have to teach lockdep about the special SRCU semantics first.
Since it doesn't even seem to do read/write locks properly that's
probably a massive undertaking.

I also doubt that it's useful, because even if we did flag this sort of
situation, it can occur across very different drivers - for example the
netronome driver using rtnl_lock() inside its debugfs files, and
mac80211 removing a completely unrelated debugfs file within
rtnl_lock().

> 
> > Similarly, nobody should be blocking in debugfs files, like we did
> > in ours, but also smsdvb_stats_read(), crtc_crc_open() look like
> > they could block for quite a while.
> 
> Blocking in the debugfs files' fops shall be fine by itself, that's
> why SRCU is used for the removal stuff.

No, it isn't fine at all now! If I have a debugfs file - like the one I
had - that could block for external events (firmware, in my case), then
*any* other debugfs_remove() in the whole system would also block
indefinitely. That's a major problem! The two other files listed above
can also block waiting for external events, afaict. I'm told that there
are more files in other wireless drivers too.

Basically, even with SRCU being used, you cannot have blocking files.
You have to treat everything as O_NONBLOCK, because if you don't a
completely unrelated debugfs_remove() will block until the file
produces data. IMHO that's completely unacceptable.

> Yes, there's only one global srcu_struct for debugfs. So far this
> hasn't been a problem and if I understand things correctly, it's also
> not the problem at hand? If it really becomes an issue, we can very
> well introduce per directory srcu_structs as you suggested.

No, this is exactly the problem. If I have one blocking file, and
remove any completely unrelated file elsewhere in the system, I need to
wait for the blocking file to have produced data. That just doesn't
scale.

Using a separate SRCU subsystem per directory will go some way, but it
would still be useful to have lockdep annotations there.

Ultimately, I'm not sure I see why one couldn't just have a
reader/writer lock per *file*, which would be the ultimate granularity
to solve this. Obviously, a blocking file has to be aborted before
being removed itself, but there's nothing that says that you can't
remove any other file - even from the same directory - while this one
is in a blocking read.

> Let me have a look
> - whether holding the RTNL lock while removing the debugfs files is
>   actually needed and
> - whether there is an easy way to spot similar scenarios and emit
>   a warning for them.
> 
> If this doesn't solve the problem, I'll have to think of a different
> way to fix this...

It solves - imho with unnecessary hardship on the caller of
debugfs_remove() - only half of the problem, namely the real deadlock.
It does nothing for the "blocking debugfs file" live-lock where forward
progress can be made as soon as the file has some data available -
which in practice in my scenario never happened as the data producer
was completely idle.

> > (*) before removing first first we'd obviously wake up and thereby
> > more or less terminate the readers first
> 
> With the current implementation, I can't see an easy way to identify
> the tasks blocking on a particular debugfs file. But maybe this is
> resolvable and the way to go here...

No, this is unrelated. If I write a blocking debugfs file, then *of
course* I need to take care that before remove it I wake up all the
readers. But that's easy because I control how that file produces data,
so I can wake up the waitq and tell my own code inside the debugfs read
to return 0 (EOF).

This would be entirely inappropriate as a general solution because
you'd have to kill the userspace process or something...

johannes

[toc] | [prev] | [next] | [standalone]

#1612889

So, please correct me if I'm wrong, there are two problems with
indefinitely blocking debugfs files' fops:

1. The one which actually hung your system:
   An indefinitely blocking debugfs_remove() while holding a lock.
   Other tasks attempting to grab that same lock get stuck as well.

2. The other one you've found, namely that the locking granularity is
   too coarse: a debugfs_remove() would get blocked by unrelated files'
   pending fops.

AFAICS, the first one can't get resolved by simply refining the blocking
granularity: a debugfs_remove() on the indefinitely blocking file would
still block as well.

But:

On Thu, Mar 30 2017, Johannes Berg wrote:

> On Thu, 2017-03-30 at 09:32 +0200, Nicolai Stange wrote:
>> 
>> I wonder if holding the RTNL during the debugfs file removal is
>> really needed. I'll try to have a look in the next couple of days.
>
> Yes, I'm pretty much convinced that it is. I considered doing a
> deferred debugfs_remove() by holding the object around, but then I
> can't be sure that I can later re-add a new object with the same
> directory name,

Ah, I see. What about making debugfs provide separate

- debugfs_remove_start(): unlink file (c.f. __debugfs_remove()), can be
  called under a lock
and
- debugfs_remove_wait(): the synchronize_srcu(), must not get called
  under any lock

?

This would solve 1.). It would still be nice to detect those situations,
i.e. calls to debugfs_remove() with some lock being held, though.

In short, I'd make calling debugfs_remove() with any lock being
held illegal.

What do you think?


> Half the thread here was about that - it's not easily doable because
> you'd have to teach lockdep about the special SRCU semantics first.
> Since it doesn't even seem to do read/write locks properly that's
> probably a massive undertaking.

I haven't looked into lockdep yet. So there is no way to ask lockdep
"is there any lock held?" from debugfs_remove() before doing the
synchonize_srcu()? 


> I also doubt that it's useful, because even if we did flag this sort of
> situation, it can occur across very different drivers - for example the
> netronome driver using rtnl_lock() inside its debugfs files, and
> mac80211 removing a completely unrelated debugfs file within
> rtnl_lock().

I'm proposing to convert the latter to a
debugfs_remove_start()/debugfs_remove_wait() pair.


>> > Similarly, nobody should be blocking in debugfs files, like we did
>> > in ours, but also smsdvb_stats_read(), crtc_crc_open() look like
>> > they could block for quite a while.
>> 
>> Blocking in the debugfs files' fops shall be fine by itself, that's
>> why SRCU is used for the removal stuff.
>
> No, it isn't fine at all now!

"Shall" in the sense of "it's a requirement" and if it isn't fulfilled,
it must be fixed. So I do agree with you here.


> If I have a debugfs file - like the one I
> had - that could block for external events (firmware, in my case), then
> *any* other debugfs_remove() in the whole system would also block
> indefinitely. That's a major problem!

Indeed.


> Ultimately, I'm not sure I see why one couldn't just have a
> reader/writer lock per *file*, which would be the ultimate granularity
> to solve this. Obviously, a blocking file has to be aborted before
> being removed itself, but there's nothing that says that you can't
> remove any other file - even from the same directory - while this one
> is in a blocking read.

When I did this, per-file reader/writer locks actuallt came to my mind
first. The problem here is that debugfs_use_file_start() must grab the
lock first and check whether the file has been deleted in the meanwhile.
But as it stands, there's nothing that would guarantee the existence of
the lock at the time it's to be taken.

Anyways, I'll have to think a little about possible solutions to mitigate
problem 2.)

Thanks,

Nicolai

[toc] | [prev] | [next] | [standalone]

#1612965

On Thu, 2017-03-30 at 12:27 +0200, Nicolai Stange wrote:
> So, please correct me if I'm wrong, there are two problems with
> indefinitely blocking debugfs files' fops:
> 
> 1. The one which actually hung your system:
>    An indefinitely blocking debugfs_remove() while holding a lock.
>    Other tasks attempting to grab that same lock get stuck as well.
> 
> 2. The other one you've found, namely that the locking granularity is
>    too coarse: a debugfs_remove() would get blocked by unrelated
> files'
>    pending fops.

No, this isn't really an accurate description of the two problems.

> AFAICS, the first one can't get resolved by simply refining the
> blocking granularity: a debugfs_remove() on the indefinitely blocking
> file would still block as well.

Correct.

The first problem - the one I ran into - is the following:

1)
A given debugfs file's .read() was waiting for some event to happen
(being a blocking file), and I was trying to debugfs_remove() some
completely unrelated file, this got stuck.
Due to me holding a lock while doing this debugfs_remove(), other tasks
*also* got stuck, but that's just a sub-problem - having the
debugfs_remove() of an unrelated file get stuck would already have been
a problem - the fact that other tasks also got stuck was just an
additional wrinkle.

Mind - this is a livelock of sorts - if the debugfs file will ever make
progress, the system can recover.

2)
There's a complete deadlock situation if this happens:

CPU1					CPU2

debugfs_file_read(file="foo")		mutex_lock(&M);
srcu_read_lock(&debugfs_srcu);		debugfs_remove(file="bar")
mutex_lock(&M);				synchronize_srcu(&debugfs_srcu)

This is intrinsically unrecoverable.

> > Yes, I'm pretty much convinced that it is. I considered doing a
> > deferred debugfs_remove() by holding the object around, but then I
> > can't be sure that I can later re-add a new object with the same
> > directory name,
> 
> Ah, I see. What about making debugfs provide separate
> 
> - debugfs_remove_start(): unlink file (c.f. __debugfs_remove()), can
> be
>   called under a lock
> and
> - debugfs_remove_wait(): the synchronize_srcu(), must not get called
>   under any lock
> 
> ?

I don't think it would really help much - the lock acquisition in my
case is in a completely different layer (cfg80211) than the code doing
debugfs_remove(), so delaying the debugfs_remove_wait() would mean
moving it somewhere else completely. Also, afaict you still have to
keep the object around until debugfs_remove_wait() has finished, so you
still have the name reuse problem.

> In short, I'd make calling debugfs_remove() with any lock being
> held illegal.
> 
> What do you think?

I think I'll stop using debugfs if that happens - too much hassle.

> > Half the thread here was about that - it's not easily doable
> > because
> > you'd have to teach lockdep about the special SRCU semantics first.
> > Since it doesn't even seem to do read/write locks properly that's
> > probably a massive undertaking.
> 
> I haven't looked into lockdep yet. So there is no way to ask lockdep
> "is there any lock held?" from debugfs_remove() before doing the
> synchonize_srcu()? 

That's probably possible, yes.

> > > > Similarly, nobody should be blocking in debugfs files, like we
> > > > did
> > > > in ours, but also smsdvb_stats_read(), crtc_crc_open() look
> > > > like
> > > > they could block for quite a while.
> > > 
> > > Blocking in the debugfs files' fops shall be fine by itself,
> > > that's why SRCU is used for the removal stuff.
> > 
> > No, it isn't fine at all now!
> 
> "Shall" in the sense of "it's a requirement" and if it isn't
> fulfilled, it must be fixed. So I do agree with you here.

Ok. So let's assume that we allow blocking (indefinitely, at least
until you remove the file) for a debugfs file - then immediately due to
the way SRCU is used you've now made some debugfs_remove() calls block
indefinitely. Not just on the same file - that'd be fine and a bug,
because before you remove a file you should wake it up - but any other
file in the system.

Even splitting it into debugfs_remove_start() and debugfs_remove_wait()
will not do anything to fix this problem - debugfs_remove_wait() would
then block forever and the task that called it will not be able to make
any forward progress, until the completely unrelated other files
created some data or got closed.

This is the core of the problem really - that you're tying completely
unrelated processes together.

Therefore, to continue using SRCU in this way means that you have to
disallow blocking debugfs files. There may not be many of those, but
any single one of them would be a problem.

If we stop using SRCU this way we can discuss how we can fix it - but
anything more coarse grained than per-file (which really makes SRCU
unsuitable) would still have the same problem one way or another. And
we haven't even addressed the deadlock situation (2 above) either.

> When I did this, per-file reader/writer locks actuallt came to my
> mind first. The problem here is that debugfs_use_file_start() must
> grab the lock first and check whether the file has been deleted in
> the meanwhile. But as it stands, there's nothing that would guarantee
> the existence of the lock at the time it's to be taken.

That seems like a strange argument to me - something has to exist for a
process to be able to look up the file, and currently the proxy also
has to exist?

So when a file is created you can allocate the proxy for it, and if you
can look up the proxy object - perhaps even using plain RCU - then you
also have the lock? IOW, instead of storing just the real_fops in
d_fsdata, you can store a small object that holds a lock and the
real_fops. You can always access that object, and lock it, but the
real_fops inside it might eventually end up NULL which you handle
through proxying. No?

johannes

[toc] | [prev] | [next] | [standalone]

#1613770

On Thu, Mar 30 2017, Johannes Berg wrote:

> On Thu, 2017-03-30 at 12:27 +0200, Nicolai Stange wrote:
>> So, please correct me if I'm wrong, there are two problems with
>> indefinitely blocking debugfs files' fops:
>> 
>> 1. The one which actually hung your system:
>>    An indefinitely blocking debugfs_remove() while holding a lock.
>>    Other tasks attempting to grab that same lock get stuck as well.
>> 
>> 2. The other one you've found, namely that the locking granularity is
>>    too coarse: a debugfs_remove() would get blocked by unrelated
>> files'
>>    pending fops.
>
> No, this isn't really an accurate description of the two problems.
>
>> AFAICS, the first one can't get resolved by simply refining the
>> blocking granularity: a debugfs_remove() on the indefinitely blocking
>> file would still block as well.
>
> Correct.
>
> The first problem - the one I ran into - is the following:
>
> 1)
> A given debugfs file's .read() was waiting for some event to happen
> (being a blocking file), and I was trying to debugfs_remove() some
> completely unrelated file, this got stuck.

I got it now. I was missing the "completely unrelated file" part.
(Admittedly, a related file would have made no sense at all -- the
remover would have been responsible to cancel any indefinite blocking in
there, as you said).

> Due to me holding a lock while doing this debugfs_remove(), other tasks
> *also* got stuck, but that's just a sub-problem - having the
> debugfs_remove() of an unrelated file get stuck would already have been
> a problem - the fact that other tasks also got stuck was just an
> additional wrinkle.
>
> Mind - this is a livelock of sorts - if the debugfs file will ever make
> progress, the system can recover.
>


> 2)
> There's a complete deadlock situation if this happens:
>
> CPU1					CPU2
>
> debugfs_file_read(file="foo")		mutex_lock(&M);
> srcu_read_lock(&debugfs_srcu);		debugfs_remove(file="bar")
> mutex_lock(&M);				synchronize_srcu(&debugfs_srcu)
>
> This is intrinsically unrecoverable.

Let's address this in a second step.


> This is the core of the problem really - that you're tying completely
> unrelated processes together.
>
> Therefore, to continue using SRCU in this way means that you have to
> disallow blocking debugfs files. There may not be many of those, but
> any single one of them would be a problem.
>
> If we stop using SRCU this way we can discuss how we can fix it - but
> anything more coarse grained than per-file (which really makes SRCU
> unsuitable) would still have the same problem one way or another. And
> we haven't even addressed the deadlock situation (2 above) either.
>
>> When I did this, per-file reader/writer locks actuallt came to my
>> mind first. The problem here is that debugfs_use_file_start() must
>> grab the lock first and check whether the file has been deleted in
>> the meanwhile. But as it stands, there's nothing that would guarantee
>> the existence of the lock at the time it's to be taken.
>
> That seems like a strange argument to me - something has to exist for a
> process to be able to look up the file, and currently the proxy also
> has to exist?

No, the proxies are created at file _open_ time and installed at the
struct file.

Rationale: there are potentially many debugfs files with only few of them
opened at a time and a proxy, i.e. a struct file_operations, is quite
large.


> So when a file is created you can allocate the proxy for it, and if you
> can look up the proxy object - perhaps even using plain RCU - then you
> also have the lock? IOW, instead of storing just the real_fops in
> d_fsdata, you can store a small object that holds a lock and the
> real_fops. You can always access that object, and lock it, but the
> real_fops inside it might eventually end up NULL which you handle
> through proxying. No?

As said, there isn't always a proxy object around.

Of course, attaching some sort of lock on a per-file basis should be
doable. I just refrained from doing it so far (and resorted to SRCU
instead) because I wasn't aware of those indefinite blockers and wanted
to avoid the additional complexity (namely avoiding use-after-frees on
that lock).

I'll work out a solution this weekend and send some RFC patches then.

Thanks for your clarifications!

Nicolai

[toc] | [prev] | [next] | [standalone]

#1613812

On Fri, 2017-03-31 at 11:03 +0200, Nicolai Stange wrote:

> > 2)
> > There's a complete deadlock situation if this happens:
> > 
> > CPU1					CPU2
> > 
> > debugfs_file_read(file="foo")		mutex_lock(&M);
> > srcu_read_lock(&debugfs_srcu);		debugfs_remove(file="
> > bar")
> > mutex_lock(&M);				synchronize_srcu(&de
> > bugfs_srcu)
> > 
> > This is intrinsically unrecoverable.
> 
> Let's address this in a second step.

I suspect that it's actually better to address both in the same step,
but whatever :)

> > That seems like a strange argument to me - something has to exist
> > for a process to be able to look up the file, and currently the
> > proxy also has to exist?
> 
> No, the proxies are created at file _open_ time and installed at the
> struct file.
> 
> Rationale: there are potentially many debugfs files with only few of
> them opened at a time and a proxy, i.e. a struct file_operations, is
> quite large.

Ok, that makes sense. But that's not really a show-stopper, is it?

You can either have a proxy or not have it at remove time, and if you
don't have one then you can remove safely, right? And if you do have a
proxy, then you have to write_lock() it.

Lookup of the proxy itself can still be protected by (S)RCU, but you
can't go into the debugfs file callbacks while you hold (S)RCU, so that
you can safely determine whether or not a proxy exists.

I'm handwaving though - there are problems here with freeing the proxy
again when you close a file. Perhaps something like
 * first, remove the pointer and wait for a grace period
 * write_lock() it to make sure nobody is still inside it
 * delete it now

works.

> I'll work out a solution this weekend and send some RFC patches then.
> 
Thanks!

johannes

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web