6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Ben Greear]

Hello,

I think this lockup is because iw is holding rtnl and wiphy mutex,
and is blocked waiting for debugfs to be closed.  Another 'cat'
program has debugfs file open, and is blocking on trying to acquire
wiphy mutex.

I think we must not acquire wiphy mutex in debugfs methods, somehow,
to resolve this deadlock.  I do not know a safe way to do that.

93434 Nov 07 01:06:35 ct523c-2103 kernel: task:iw              state:D stack:
       0     pid:342999 tgid:342999 ppid:342998 flags:0x00004002
93435 Nov 07 01:06:35 ct523c-2103 kernel: Call Trace:
93436 Nov 07 01:06:35 ct523c-2103 kernel:  <TASK>
93437 Nov 07 01:06:35 ct523c-2103 kernel:  __schedule+0x377/0xa10
93438 Nov 07 01:06:35 ct523c-2103 kernel:  ? sched_clock_cpu+0xb/0x180
93439 Nov 07 01:06:35 ct523c-2103 kernel:  schedule+0x29/0xc0
93440 Nov 07 01:06:35 ct523c-2103 kernel:  schedule_timeout+0x136/0x150
93441 Nov 07 01:06:35 ct523c-2103 kernel:  ? d_walk+0x2a2/0x2b0
93442 Nov 07 01:06:35 ct523c-2103 kernel:  wait_for_completion+0x7d/0x130
93443 Nov 07 01:06:35 ct523c-2103 kernel:  remove_one+0x52/0x70
93444 Nov 07 01:06:35 ct523c-2103 kernel:  simple_recursive_removal+0x1e3/0x280
93445 Nov 07 01:06:35 ct523c-2103 kernel:  ? start_creating.part.0+0x170/0x170
93446 Nov 07 01:06:35 ct523c-2103 kernel:  debugfs_remove+0x3b/0x60
93447 Nov 07 01:06:35 ct523c-2103 kernel:  ieee80211_debugfs_remove_netdev+0x15//
       0x30 [mac80211]
93448 Nov 07 01:06:35 ct523c-2103 kernel:  ieee80211_teardown_sdata+0x13/0x50
       [mac80211]
93449 Nov 07 01:06:35 ct523c-2103 kernel:  unregister_netdevice_many_notify+
       0x3a8/0x7c0
93450 Nov 07 01:06:35 ct523c-2103 kernel:  unregister_netdevice_queue+0x84/0xc0
93451 Nov 07 01:06:35 ct523c-2103 kernel:  _cfg80211_unregister_wdev+0x1e7/
       0x230 [cfg80211]
93452 Nov 07 01:06:35 ct523c-2103 kernel:  ieee80211_if_remove+0x8b/0xe0
       [mac80211]
93453 Nov 07 01:06:35 ct523c-2103 kernel:  ieee80211_del_iface+0xc/0x10
       [mac80211]
93454 Nov 07 01:06:35 ct523c-2103 kernel:  cfg80211_remove_virtual_intf+0x28/
       0xd0 [cfg80211]
93455 Nov 07 01:06:35 ct523c-2103 kernel:  genl_family_rcv_msg_doit+0xd1/0x120
93456 Nov 07 01:06:35 ct523c-2103 kernel:  genl_rcv_msg+0x17b/0x290
93457 Nov 07 01:06:35 ct523c-2103 kernel:  ? __cfg80211_wdev_from_attrs+0x290/
       0x290 [cfg80211]
93458 Nov 07 01:06:35 ct523c-2103 kernel:  ? nl80211_stop_ap+0x30/0x30
       [cfg80211]
93459 Nov 07 01:06:35 ct523c-2103 kernel:  ? nlmsg_trim+0x20/0x20 [cfg80211]
93460 Nov 07 01:06:35 ct523c-2103 kernel:  ? genl_family_rcv_msg_dumpit+0xf0/
       0xf0
93461 Nov 07 01:06:35 ct523c-2103 kernel:  netlink_rcv_skb+0x4f/0x100
93462 Nov 07 01:06:35 ct523c-2103 kernel:  genl_rcv+0x1f/0x30
93463 Nov 07 01:06:35 ct523c-2103 kernel:  netlink_unicast+0x18b/0x280
93464 Nov 07 01:06:35 ct523c-2103 kernel:  netlink_sendmsg+0x24b/0x4c0
93465 Nov 07 01:06:35 ct523c-2103 kernel:  __sock_sendmsg+0x33/0x60
93466 Nov 07 01:06:35 ct523c-2103 kernel:  ____sys_sendmsg+0x229/0x2a0
93467 Nov 07 01:06:35 ct523c-2103 kernel:  ? copy_msghdr_from_user+0x68/0xa0
93468 Nov 07 01:06:35 ct523c-2103 kernel:  ___sys_sendmsg+0x81/0xc0
93469 Nov 07 01:06:35 ct523c-2103 kernel:  ? do_fault+0x238/0x450
93470 Nov 07 01:06:35 ct523c-2103 kernel:  __sys_sendmsg+0x52/0xa0
93471 Nov 07 01:06:35 ct523c-2103 kernel:  do_syscall_64+0x3b/0x110

gdb) l *(remove_one+0x52)
0xffffffff81540d22 is in remove_one (/home/greearb/git/linux-6.7.dev.y/fs/debugfs/inode.c:735).
730		smp_mb();
731		fsd = READ_ONCE(dentry->d_fsdata);
732		if ((unsigned long)fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)
733			return;
734		if (!refcount_dec_and_test(&fsd->active_users))
735			wait_for_completion(&fsd->active_users_drained);
736	}
737	
738	static void remove_one(struct dentry *victim)
739	{

(gdb) l *(wait_for_completion+0x7d)
0xffffffff81dbc47d is in wait_for_completion (/home/greearb/git/linux-6.7.dev.y/kernel/sched/completion.c:96).
91				}
92				__prepare_to_swait(&x->wait, &wait);
93				__set_current_state(state);
94				raw_spin_unlock_irq(&x->wait.lock);
95				timeout = action(timeout);
96				raw_spin_lock_irq(&x->wait.lock);
97			} while (!x->done && timeout);
98			__finish_swait(&x->wait, &wait);
99			if (!x->done)
100				return timeout;
(gdb)


I noticed a blocked 'cat' process.  This one has debugfs open, and is blocked on wiphy mutex:

101118 Nov 07 13:50:49 ct523c-2103 kernel: task:cat             state:D stack:
        0     pid:343014 tgid:343014 ppid:342995 flags:0x00000002
101119 Nov 07 13:50:49 ct523c-2103 kernel: Call Trace:
101120 Nov 07 13:50:49 ct523c-2103 kernel:  <TASK>
101121 Nov 07 13:50:49 ct523c-2103 kernel:  __schedule+0x377/0xa10
101122 Nov 07 13:50:49 ct523c-2103 kernel:  schedule+0x29/0xc0
101123 Nov 07 13:50:49 ct523c-2103 kernel:  schedule_preempt_disabled+0xc/0x20
101124 Nov 07 13:50:49 ct523c-2103 kernel:  __mutex_lock.constprop.0+0x34d/0x760
101125 Nov 07 13:50:49 ct523c-2103 kernel:  ?
        ieee80211_if_fmt_dot11MeshMaxRetries+0x20/0x20 [mac80211]
101126 Nov 07 13:50:49 ct523c-2103 kernel:  ieee80211_if_read_sdata+0x3f/0xb0
        [mac80211]
101127 Nov 07 13:50:49 ct523c-2103 kernel:  ? __pte_offset_map+0x12/0x160
101128 Nov 07 13:50:49 ct523c-2103 kernel:  ? __pte_offset_map_lock+0x8e/0x100
101129 Nov 07 13:50:49 ct523c-2103 kernel:  ? folio_add_new_anon_rmap+0x48/0xd0
101130 Nov 07 13:50:49 ct523c-2103 kernel:  ? set_ptes.constprop.0+0x1e/0xa0
101131 Nov 07 13:50:49 ct523c-2103 kernel:  ? do_anonymous_page+0x2b2/0x3d0
101132 Nov 07 13:50:49 ct523c-2103 kernel:  ? __handle_mm_fault+0xa0f/0xd10

Method in question is this:

static ssize_t ieee80211_if_read_sdata(
	struct ieee80211_sub_if_data *sdata,
	char __user *userbuf,
	size_t count, loff_t *ppos,
	ssize_t (*format)(const struct ieee80211_sub_if_data *sdata, char *, int))
{
	char buf[200];
	ssize_t ret = -EINVAL;

	wiphy_lock(sdata->local->hw.wiphy);
	ret = (*format)(sdata, buf, sizeof(buf));
	wiphy_unlock(sdata->local->hw.wiphy);


Other programs are blocked trying to get rtnl or wiphy locks, but I think they
are not the issue...  Full logs available if someone wants to see it...


Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Johannes Berg]

On Tue, 2023-11-07 at 14:08 -0800, Ben Greear wrote:
> Hello,
> 
> I think this lockup is because iw is holding rtnl and wiphy mutex,
> and is blocked waiting for debugfs to be closed.  Another 'cat'
> program has debugfs file open, and is blocking on trying to acquire
> wiphy mutex.
> 
> I think we must not acquire wiphy mutex in debugfs methods, somehow,
> to resolve this deadlock.  I do not know a safe way to do that.

Hmm. I almost want to say "don't do that then", but I guess you're just
randomly accessing debugfs files.

I guess we can at least make the mutex acquisition in debugfs killable
(or interruptible), so you can recover from this.

But fundamentally this is probably not really even a new issue.

I don't know how to interrupt a specific task that's stuck in a specific
debugfs file though, e.g. when removing them.

johannes

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Ben Greear]

On 11/8/23 2:31 AM, Johannes Berg wrote:
> On Tue, 2023-11-07 at 14:08 -0800, Ben Greear wrote:
>> Hello,
>>
>> I think this lockup is because iw is holding rtnl and wiphy mutex,
>> and is blocked waiting for debugfs to be closed.  Another 'cat'
>> program has debugfs file open, and is blocking on trying to acquire
>> wiphy mutex.
>>
>> I think we must not acquire wiphy mutex in debugfs methods, somehow,
>> to resolve this deadlock.  I do not know a safe way to do that.
> 
> Hmm. I almost want to say "don't do that then", but I guess you're just
> randomly accessing debugfs files.
> 
> I guess we can at least make the mutex acquisition in debugfs killable
> (or interruptible), so you can recover from this.

If we can detect that the phy is going away in debugfs, then we could
return early before attempting the lock?  That would catch most things,
I guess, but still a potential race since I guess we'd have to do that check
w/out locks.  Can we do a try-mutex-lock, if not acquired, return if wiphy-going-away,
else sleep a bit, try again?

> 
> But fundamentally this is probably not really even a new issue.
> 
> I don't know how to interrupt a specific task that's stuck in a specific
> debugfs file though, e.g. when removing them.

Or, can we grab rtnl before we even open the debugfs file, like in the .open method?

Or can we remove the debugfs files after rtnl but before we lock the wiphy mutex
in the destruction path?

I have been running similar code for...like 15 years, and haven't seen this particular
deadlock before, so I think it is at least exacerbated by the locking changes.  Or maybe
I had particularly bad luck yesterday....

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Johannes Berg]

On Wed, 2023-11-08 at 07:07 -0800, Ben Greear wrote:
> On 11/8/23 2:31 AM, Johannes Berg wrote:
> > On Tue, 2023-11-07 at 14:08 -0800, Ben Greear wrote:
> > > Hello,
> > > 
> > > I think this lockup is because iw is holding rtnl and wiphy mutex,
> > > and is blocked waiting for debugfs to be closed.  Another 'cat'
> > > program has debugfs file open, and is blocking on trying to acquire
> > > wiphy mutex.
> > > 
> > > I think we must not acquire wiphy mutex in debugfs methods, somehow,
> > > to resolve this deadlock.  I do not know a safe way to do that.
> > 
> > Hmm. I almost want to say "don't do that then", but I guess you're just
> > randomly accessing debugfs files.
> > 
> > I guess we can at least make the mutex acquisition in debugfs killable
> > (or interruptible), so you can recover from this.
> 
> If we can detect that the phy is going away in debugfs, then we could
> return early before attempting the lock?  That would catch most things,
> I guess,
> 

I don't think it would, it would still get locked on the mutex first.

>  but still a potential race since I guess we'd have to do that check
> w/out locks.  Can we do a try-mutex-lock, if not acquired, return if wiphy-going-away,
> else sleep a bit, try again?

That's kind of awful though? And it's not just the wiphy going away, a
lot of the debugfs files can go away individually (per station, per
link, per key even!).

So really what you'd need is a debugfs-level infrastructure to "send a
signal to all the things that are keeping the file open"? I suppose that
could even be done, in theory, but not in wifi by itself.

> Or, can we grab rtnl before we even open the debugfs file, like in the .open method?

Not RTNL, but rather wiphy mutex, but the question still stands - but
no, the open method has the same problem. If we acquire it there, it
still goes through the proxy fops in debugfs, so it'll still wait for it
to be done. It'll just shift the problem to another place.

> Or can we remove the debugfs files after rtnl but before we lock the wiphy mutex
> in the destruction path?

For some maybe yes, but for a lot of them like link/sta/key removal not
really.

> I have been running similar code for...like 15 years, and haven't seen this particular
> deadlock before, so I think it is at least exacerbated by the locking changes.  Or maybe
> I had particularly bad luck yesterday....

Oh, it almost certainly did get at least worse or perhaps introduced by
(a) moving everything to a single lock and (b) moving debugfs file
removal under the lock.

johannes

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Ben Greear]

On 11/8/23 7:44 AM, Johannes Berg wrote:
> On Wed, 2023-11-08 at 07:07 -0800, Ben Greear wrote:
>> On 11/8/23 2:31 AM, Johannes Berg wrote:
>>> On Tue, 2023-11-07 at 14:08 -0800, Ben Greear wrote:
>>>> Hello,
>>>>
>>>> I think this lockup is because iw is holding rtnl and wiphy mutex,
>>>> and is blocked waiting for debugfs to be closed.  Another 'cat'
>>>> program has debugfs file open, and is blocking on trying to acquire
>>>> wiphy mutex.
>>>>
>>>> I think we must not acquire wiphy mutex in debugfs methods, somehow,
>>>> to resolve this deadlock.  I do not know a safe way to do that.
>>>
>>> Hmm. I almost want to say "don't do that then", but I guess you're just
>>> randomly accessing debugfs files.
>>>
>>> I guess we can at least make the mutex acquisition in debugfs killable
>>> (or interruptible), so you can recover from this.
>>
>> If we can detect that the phy is going away in debugfs, then we could
>> return early before attempting the lock?  That would catch most things,
>> I guess,
>>
> 
> I don't think it would, it would still get locked on the mutex first.
> 
>>   but still a potential race since I guess we'd have to do that check
>> w/out locks.  Can we do a try-mutex-lock, if not acquired, return if wiphy-going-away,
>> else sleep a bit, try again?
> 
> That's kind of awful though? And it's not just the wiphy going away, a
> lot of the debugfs files can go away individually (per station, per
> link, per key even!).

 From the backtrace in the removal logic, it seems that something waits
for a debugfs file to be closed.  Maybe the logic attempting to get the
mutex in debugfs can check if file is waiting to be deleted,
combined with a try-mutex-lock logic, and bail out that way?

Thanks,
Ben

> 
> So really what you'd need is a debugfs-level infrastructure to "send a
> signal to all the things that are keeping the file open"? I suppose that
> could even be done, in theory, but not in wifi by itself.
> 
>> Or, can we grab rtnl before we even open the debugfs file, like in the .open method?
> 
> Not RTNL, but rather wiphy mutex, but the question still stands - but
> no, the open method has the same problem. If we acquire it there, it
> still goes through the proxy fops in debugfs, so it'll still wait for it
> to be done. It'll just shift the problem to another place.
> 
>> Or can we remove the debugfs files after rtnl but before we lock the wiphy mutex
>> in the destruction path?
> 
> For some maybe yes, but for a lot of them like link/sta/key removal not
> really.
> 
>> I have been running similar code for...like 15 years, and haven't seen this particular
>> deadlock before, so I think it is at least exacerbated by the locking changes.  Or maybe
>> I had particularly bad luck yesterday....
> 
> Oh, it almost certainly did get at least worse or perhaps introduced by
> (a) moving everything to a single lock and (b) moving debugfs file
> removal under the lock.
> 
> johannes
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Johannes Berg]

On Wed, 2023-11-08 at 07:55 -0800, Ben Greear wrote:
> On 11/8/23 7:44 AM, Johannes Berg wrote:
> > On Wed, 2023-11-08 at 07:07 -0800, Ben Greear wrote:
> > > On 11/8/23 2:31 AM, Johannes Berg wrote:
> > > > On Tue, 2023-11-07 at 14:08 -0800, Ben Greear wrote:
> > > > > Hello,
> > > > > 
> > > > > I think this lockup is because iw is holding rtnl and wiphy mutex,
> > > > > and is blocked waiting for debugfs to be closed.  Another 'cat'
> > > > > program has debugfs file open, and is blocking on trying to acquire
> > > > > wiphy mutex.
> > > > > 
> > > > > I think we must not acquire wiphy mutex in debugfs methods, somehow,
> > > > > to resolve this deadlock.  I do not know a safe way to do that.
> > > > 
> > > > Hmm. I almost want to say "don't do that then", but I guess you're just
> > > > randomly accessing debugfs files.
> > > > 
> > > > I guess we can at least make the mutex acquisition in debugfs killable
> > > > (or interruptible), so you can recover from this.
> > > 
> > > If we can detect that the phy is going away in debugfs, then we could
> > > return early before attempting the lock?  That would catch most things,
> > > I guess,
> > > 
> > 
> > I don't think it would, it would still get locked on the mutex first.
> > 
> > >   but still a potential race since I guess we'd have to do that check
> > > w/out locks.  Can we do a try-mutex-lock, if not acquired, return if wiphy-going-away,
> > > else sleep a bit, try again?
> > 
> > That's kind of awful though? And it's not just the wiphy going away, a
> > lot of the debugfs files can go away individually (per station, per
> > link, per key even!).
> 
>  From the backtrace in the removal logic, it seems that something waits
> for a debugfs file to be closed.

Yes, debugfs remove waits for it to no longer have active users, but
that cannot succeed because the users are blocked on acquiring the
mutex.

> Maybe the logic attempting to get the
> mutex in debugfs can check if file is waiting to be deleted,
> combined with a try-mutex-lock logic, and bail out that way?

I don't know if there's a way to check that, but I'm also not sure how
you'd even implement that?

johannes

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Johannes Berg]

On Wed, 2023-11-08 at 16:44 +0100, Johannes Berg wrote:
> 
> > I have been running similar code for...like 15 years, and haven't seen this particular
> > deadlock before, so I think it is at least exacerbated by the locking changes.  Or maybe
> > I had particularly bad luck yesterday....
> 
> Oh, it almost certainly did get at least worse or perhaps introduced by
> (a) moving everything to a single lock and (b) moving debugfs file
> removal under the lock.
> 

FWIW, the debugfs remove was under lock before, but we didn't acquire
much locks in the debugfs itself (only sta mtx and only in aql write,
and that mutex isn't held in remove) ... So indeed it couldn't happen.

I added that to call into drivers with the wiphy mutex though, can't
really remove that, I think.

johannes

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Benjamin Berg]

On Wed, 2023-11-08 at 17:07 +0100, Johannes Berg wrote:
> >  From the backtrace in the removal logic, it seems that something waits
> > for a debugfs file to be closed.
> 
> Yes, debugfs remove waits for it to no longer have active users, but
> that cannot succeed because the users are blocked on acquiring the
> mutex.
> 
> > Maybe the logic attempting to get the
> > mutex in debugfs can check if file is waiting to be deleted,
> > combined with a try-mutex-lock logic, and bail out that way?
> 
> I don't know if there's a way to check that, but I'm also not sure how
> you'd even implement that?

Is it likely that we have lock contention for debugfs operations?

If it is relatively unlikely, then maybe just doing a mutex_trylock()
and immediately failing the operation with -EAGAIN could be a solution?
Obviously userspace would need some retry logic, but that is simple and
it could solve the delete problem.

Benjamin

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Ben Greear]

On 11/8/23 08:07, Johannes Berg wrote:
> On Wed, 2023-11-08 at 07:55 -0800, Ben Greear wrote:
>> On 11/8/23 7:44 AM, Johannes Berg wrote:
>>> On Wed, 2023-11-08 at 07:07 -0800, Ben Greear wrote:
>>>> On 11/8/23 2:31 AM, Johannes Berg wrote:
>>>>> On Tue, 2023-11-07 at 14:08 -0800, Ben Greear wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I think this lockup is because iw is holding rtnl and wiphy mutex,
>>>>>> and is blocked waiting for debugfs to be closed.  Another 'cat'
>>>>>> program has debugfs file open, and is blocking on trying to acquire
>>>>>> wiphy mutex.
>>>>>>
>>>>>> I think we must not acquire wiphy mutex in debugfs methods, somehow,
>>>>>> to resolve this deadlock.  I do not know a safe way to do that.
>>>>>
>>>>> Hmm. I almost want to say "don't do that then", but I guess you're just
>>>>> randomly accessing debugfs files.
>>>>>
>>>>> I guess we can at least make the mutex acquisition in debugfs killable
>>>>> (or interruptible), so you can recover from this.
>>>>
>>>> If we can detect that the phy is going away in debugfs, then we could
>>>> return early before attempting the lock?  That would catch most things,
>>>> I guess,
>>>>
>>>
>>> I don't think it would, it would still get locked on the mutex first.
>>>
>>>>    but still a potential race since I guess we'd have to do that check
>>>> w/out locks.  Can we do a try-mutex-lock, if not acquired, return if wiphy-going-away,
>>>> else sleep a bit, try again?
>>>
>>> That's kind of awful though? And it's not just the wiphy going away, a
>>> lot of the debugfs files can go away individually (per station, per
>>> link, per key even!).
>>
>>   From the backtrace in the removal logic, it seems that something waits
>> for a debugfs file to be closed.
> 
> Yes, debugfs remove waits for it to no longer have active users, but
> that cannot succeed because the users are blocked on acquiring the
> mutex.
> 
>> Maybe the logic attempting to get the
>> mutex in debugfs can check if file is waiting to be deleted,
>> combined with a try-mutex-lock logic, and bail out that way?
> 
> I don't know if there's a way to check that, but I'm also not sure how
> you'd even implement that?

This code is new to me, but we have a file object in debugfs callbacks,
and this method appears to get dentry from a file object:

static inline struct dentry *file_dentry(const struct file *file)
{
	return d_real(file->f_path.dentry, file_inode(file));
}


In the delete path, we have a dentry object:

void debugfs_remove(struct dentry *dentry)
{
	if (IS_ERR_OR_NULL(dentry))
		return;

	simple_pin_fs(&debug_fs_type, &debugfs_mount, &debugfs_mount_count);
	simple_recursive_removal(dentry, remove_one);
	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
}
EXPORT_SYMBOL_GPL(debugfs_remove);


We could add a wiphy_trylock method, similar to this:

static inline void wiphy_lock(struct wiphy *wiphy)
         __acquires(&wiphy->mtx)
{
         mutex_lock(&wiphy->mtx);
         __acquire(&wiphy->mtx);
}


But using mutex_trylock instead.


This method appears to actually set a S_DEAD flag in the dentry, so maybe
we just check that flag in the mutex_trylock failed to acquire path
in the debugfs read?

void simple_recursive_removal(struct dentry *dentry,
                               void (*callback)(struct dentry *))
{
	struct dentry *this = dget(dentry);
	while (true) {
		struct dentry *victim = NULL, *child;
		struct inode *inode = this->d_inode;

		inode_lock(inode);
		if (d_is_dir(this))
			inode->i_flags |= S_DEAD;


Let me know what you think on that...

Thanks,
Ben

> 
> johannes
> 

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Ben Greear]

On 11/8/23 09:39, Benjamin Berg wrote:
> On Wed, 2023-11-08 at 17:07 +0100, Johannes Berg wrote:
>>>   From the backtrace in the removal logic, it seems that something waits
>>> for a debugfs file to be closed.
>>
>> Yes, debugfs remove waits for it to no longer have active users, but
>> that cannot succeed because the users are blocked on acquiring the
>> mutex.
>>
>>> Maybe the logic attempting to get the
>>> mutex in debugfs can check if file is waiting to be deleted,
>>> combined with a try-mutex-lock logic, and bail out that way?
>>
>> I don't know if there's a way to check that, but I'm also not sure how
>> you'd even implement that?
> 
> Is it likely that we have lock contention for debugfs operations?
> 
> If it is relatively unlikely, then maybe just doing a mutex_trylock()
> and immediately failing the operation with -EAGAIN could be a solution?
> Obviously userspace would need some retry logic, but that is simple and
> it could solve the delete problem.

That is pretty nasty to expect each and every user-space app anywhere to suddenly
know that file operations are randomly unreliable...

I think we can do this where we only return no useful file data when we are actually
in teardown phase...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Johannes Berg]

On Wed, 2023-11-08 at 09:44 -0800, Ben Greear wrote:
> 
> This method appears to actually set a S_DEAD flag in the dentry,
> 

A *dir* entry.

>  so maybe
> we just check that flag in the mutex_trylock failed to acquire path
> in the debugfs read?
> 
> void simple_recursive_removal(struct dentry *dentry,
>                                void (*callback)(struct dentry *))
> {
> 	struct dentry *this = dget(dentry);
> 	while (true) {
> 		struct dentry *victim = NULL, *child;
> 		struct inode *inode = this->d_inode;
> 
> 		inode_lock(inode);
> 		if (d_is_dir(this))
> 			inode->i_flags |= S_DEAD;
> 

But even if you did that, I'm not sure what you'd do? trylock and sleep,
and abort if you find it's being removed? That all sounds super awkward.

johannes

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Ben Greear]

On 11/8/23 10:43, Johannes Berg wrote:
> On Wed, 2023-11-08 at 09:44 -0800, Ben Greear wrote:
>>
>> This method appears to actually set a S_DEAD flag in the dentry,
>>
> 
> A *dir* entry.
> 
>>   so maybe
>> we just check that flag in the mutex_trylock failed to acquire path
>> in the debugfs read?
>>
>> void simple_recursive_removal(struct dentry *dentry,
>>                                 void (*callback)(struct dentry *))
>> {
>> 	struct dentry *this = dget(dentry);
>> 	while (true) {
>> 		struct dentry *victim = NULL, *child;
>> 		struct inode *inode = this->d_inode;
>>
>> 		inode_lock(inode);
>> 		if (d_is_dir(this))
>> 			inode->i_flags |= S_DEAD;
>>
> 
> But even if you did that, I'm not sure what you'd do? trylock and sleep,
> and abort if you find it's being removed? That all sounds super awkward.

Here is my compile-only tested approach.  It only tries to fix one particular
locking case in debugfs, so I'm sure it is not yet complete.  I believe I
need to add that new flag in the fs.h, as S_DEAD is only for directories,
if I understand the code properly.


diff --git a/fs/libfs.c b/fs/libfs.c
index abe2b5a40ba1..5b420e019ad4 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -520,6 +520,7 @@ void simple_recursive_removal(struct dentry *dentry,
                 inode_lock(inode);
                 if (d_is_dir(this))
                         inode->i_flags |= S_DEAD;
+               inode->i_flags |= S_GOING_AWAY;
                 while ((child = find_next_child(this, victim)) == NULL) {
                         // kill and ascend
                         // update metadata while it's still locked
@@ -530,6 +531,7 @@ void simple_recursive_removal(struct dentry *dentry,
                         this = this->d_parent;
                         inode = this->d_inode;
                         inode_lock(inode);
+                       inode->i_flags |= S_GOING_AWAY;
                         if (simple_positive(victim)) {
                                 d_invalidate(victim);   // avoid lost mounts
                                 if (d_is_dir(victim))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 98b7a7a8c42e..b76eafe31427 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2116,6 +2116,7 @@ struct super_operations {
  #define S_CASEFOLD     (1 << 15) /* Casefolded file */
  #define S_VERITY       (1 << 16) /* Verity file (using fs/verity/) */
  #define S_KERNEL_FILE  (1 << 17) /* File is in use by the kernel (eg. fs/cachefiles) */
+#define S_GOING_AWAY   (1 << 18) /* dentry is trying to be deleted */

  /*
   * Note that nosuid etc flags are inode-specific: setting some file-system
@@ -2163,6 +2164,7 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
  #define IS_ENCRYPTED(inode)    ((inode)->i_flags & S_ENCRYPTED)
  #define IS_CASEFOLDED(inode)   ((inode)->i_flags & S_CASEFOLD)
  #define IS_VERITY(inode)       ((inode)->i_flags & S_VERITY)
+#define IS_INODE_GOINGAWAY(inode) ((inode)->i_flags & S_GOING_AWAY)

  #define IS_WHITEOUT(inode)     (S_ISCHR(inode->i_mode) && \
                                  (inode)->i_rdev == WHITEOUT_DEV)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 34fbefad7652..b4f3bf1c6b19 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5881,6 +5881,33 @@ static inline void wiphy_lock(struct wiphy *wiphy)
         __acquire(&wiphy->mtx);
  }

+/**
+ * wiphy_trylock - try to lock the wiphy
+ * @wiphy: the wiphy to lock
+ *
+ * This is needed around registering and unregistering netdevs that
+ * aren't created through cfg80211 calls, since that requires locking
+ * in cfg80211 when the notifiers is called, but that cannot
+ * differentiate which way it's called.
+ *
+ * It can also be used by drivers for their own purposes.
+ *
+ * When cfg80211 ops are called, the wiphy is already locked.
+ *
+ * Note that this makes sure that no workers that have been queued
+ * with wiphy_queue_work() are running.
+ *
+ * Returns 0 if lock failed, non-zero otherwise.
+ */
+static inline int wiphy_trylock(struct wiphy *wiphy)
+       __acquires(&wiphy->mtx)
+{
+       int rv = mutex_trylock(&wiphy->mtx);
+       if (rv)
+               __acquire(&wiphy->mtx);
+       return rv;
+}
+
  /**
   * wiphy_unlock - unlock the wiphy again
   * @wiphy: the wiphy to unlock
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index ec91e131b29e..c22697969973 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -23,15 +23,31 @@
  #include "driver-ops.h"

  static ssize_t ieee80211_if_read_sdata(
+       struct file* file,
         struct ieee80211_sub_if_data *sdata,
         char __user *userbuf,
         size_t count, loff_t *ppos,
         ssize_t (*format)(const struct ieee80211_sub_if_data *sdata, char *, int))
  {
-       char buf[200];
         ssize_t ret = -EINVAL;
-
-       wiphy_lock(sdata->local->hw.wiphy);
+       struct inode* f_inode;
+       char buf[200];
+       int rv;
+
+       while (true) {
+               rv = wiphy_trylock(sdata->local->hw.wiphy);
+               if (rv) {
+                       break; /* we have the lock */
+               } else {
+                       /* failed to lock, see if we are being deleted. */
+                       f_inode = file_inode(file);
+                       if (IS_INODE_GOINGAWAY(f_inode) || IS_DEADDIR(f_inode)) {
+                               return -ESTALE;
+                       }
+                       /* Sleep for a bit so we don't busy-spin too hard on try-lock */
+                       msleep(1);
+               }
+       }
         ret = (*format)(sdata, buf, sizeof(buf));
         wiphy_unlock(sdata->local->hw.wiphy);

@@ -173,7 +189,7 @@ static ssize_t ieee80211_if_read_##name(struct file *file,          \
                                         char __user *userbuf,           \
                                         size_t count, loff_t *ppos)     \
  {                                                                      \
-       return ieee80211_if_read_sdata(file->private_data,              \
+       return ieee80211_if_read_sdata(file, file->private_data,        \
                                        userbuf, count, ppos,            \
                                        ieee80211_if_fmt_##name);        \
  }

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: 6.7.0-rc1 + hacks deadlock bug, wifi netdev delete + cat of debugfs file.

[Johannes Berg]

On Wed, 2023-11-08 at 12:04 -0800, Ben Greear wrote:
> On 11/8/23 10:43, Johannes Berg wrote:
> > On Wed, 2023-11-08 at 09:44 -0800, Ben Greear wrote:
> > > 
> > > This method appears to actually set a S_DEAD flag in the dentry,
> > > 
> > 
> > A *dir* entry.
> > 
> > >   so maybe
> > > we just check that flag in the mutex_trylock failed to acquire path
> > > in the debugfs read?
> > > 
> > > void simple_recursive_removal(struct dentry *dentry,
> > >                                 void (*callback)(struct dentry *))
> > > {
> > > 	struct dentry *this = dget(dentry);
> > > 	while (true) {
> > > 		struct dentry *victim = NULL, *child;
> > > 		struct inode *inode = this->d_inode;
> > > 
> > > 		inode_lock(inode);
> > > 		if (d_is_dir(this))
> > > 			inode->i_flags |= S_DEAD;
> > > 
> > 
> > But even if you did that, I'm not sure what you'd do? trylock and sleep,
> > and abort if you find it's being removed? That all sounds super awkward.
> 
> Here is my compile-only tested approach.  It only tries to fix one particular
> locking case in debugfs, so I'm sure it is not yet complete.  I believe I
> need to add that new flag in the fs.h, as S_DEAD is only for directories,
> if I understand the code properly.

[snip code]

Sure, it's just software. You can do whatever you want with it. But ...
there's no way anything even similar to this will ever go upstream.

johannes