A race condition between debugfs and seq_file operation

A race condition between debugfs and seq_file operation

[Lisa Du]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1192 bytes --]

Hi, All
Recently I met one race condition related to debugfs.

Take an example from ion.c in kernel3.14:
static int ion_debug_client_open(struct inode *inode, struct file *file)
{
	return single_open(file, ion_debug_client_show, inode->i_private);
}

static const struct file_operations debug_client_fops = {
	.open = ion_debug_client_open,
	.read = seq_read,
	.llseek = seq_lseek,
	.release = single_release,
};
client->debug_root = debugfs_create_file(client->display_name, 0664,
					dev->clients_debug_root,
					client, &debug_client_fops);

I find during I read the debugfs node, driver can do debugfs_remove_recursive(dentry);
Is it expected?  

In this case, when do the seq_file read, it grabs the seq_file->lock;
While in debugfs_remove_recursive(), it graps "parent->d_inode->i_mutex".
So there seems no protection between the ion_debug_client_show() and debugfs_remove_recursive().

Please let me know if I didn't describe the issue clear.
Would you help to comment if there's method to avoid such issue?

Best Regards.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: A race condition between debugfs and seq_file operation

[gregkh]

On Mon, Jun 08, 2015 at 04:28:10AM +0000, Lisa Du wrote:
> Hi, All
> Recently I met one race condition related to debugfs.
> 
> Take an example from ion.c in kernel3.14:
> static int ion_debug_client_open(struct inode *inode, struct file *file)
> {
> 	return single_open(file, ion_debug_client_show, inode->i_private);
> }
> 
> static const struct file_operations debug_client_fops = {
> 	.open = ion_debug_client_open,
> 	.read = seq_read,
> 	.llseek = seq_lseek,
> 	.release = single_release,
> };
> client->debug_root = debugfs_create_file(client->display_name, 0664,
> 					dev->clients_debug_root,
> 					client, &debug_client_fops);
> 
> I find during I read the debugfs node, driver can do debugfs_remove_recursive(dentry);
> Is it expected?  

Yes.  Well, not "expected", but a mess, yes.

Removing debugfs files are known to have lots of races, this isn't the
only one :(

thanks,

greg k-h

RE: A race condition between debugfs and seq_file operation

[Lisa Du]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1453 bytes --]

> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: 2015Äê6ÔÂ10ÈÕ 5:12
> To: Lisa Du
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: A race condition between debugfs and seq_file operation
> 
> On Mon, Jun 08, 2015 at 04:28:10AM +0000, Lisa Du wrote:
> > Hi, All
> > Recently I met one race condition related to debugfs.
> >
> > Take an example from ion.c in kernel3.14:
> > static int ion_debug_client_open(struct inode *inode, struct file
> > *file) {
> > 	return single_open(file, ion_debug_client_show, inode->i_private); }
> >
> > static const struct file_operations debug_client_fops = {
> > 	.open = ion_debug_client_open,
> > 	.read = seq_read,
> > 	.llseek = seq_lseek,
> > 	.release = single_release,
> > };
> > client->debug_root = debugfs_create_file(client->display_name, 0664,
> > 					dev->clients_debug_root,
> > 					client, &debug_client_fops);
> >
> > I find during I read the debugfs node, driver can do
> > debugfs_remove_recursive(dentry); Is it expected?
> 
> Yes.  Well, not "expected", but a mess, yes.
> 
> Removing debugfs files are known to have lots of races, this isn't the only
> one :(
Thanks for the reply! 
Not sure if there is any plan to resolve such races in the future?
> 
> thanks,
> 
> greg k-h
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: A race condition between debugfs and seq_file operation

[gregkh]

On Wed, Jun 10, 2015 at 05:00:03AM +0000, Lisa Du wrote:
> > -----Original Message-----
> > From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> > Sent: 2015年6月10日 5:12
> > To: Lisa Du
> > Cc: linux-kernel@vger.kernel.org
> > Subject: Re: A race condition between debugfs and seq_file operation
> > 
> > On Mon, Jun 08, 2015 at 04:28:10AM +0000, Lisa Du wrote:
> > > Hi, All
> > > Recently I met one race condition related to debugfs.
> > >
> > > Take an example from ion.c in kernel3.14:
> > > static int ion_debug_client_open(struct inode *inode, struct file
> > > *file) {
> > > 	return single_open(file, ion_debug_client_show, inode->i_private); }
> > >
> > > static const struct file_operations debug_client_fops = {
> > > 	.open = ion_debug_client_open,
> > > 	.read = seq_read,
> > > 	.llseek = seq_lseek,
> > > 	.release = single_release,
> > > };
> > > client->debug_root = debugfs_create_file(client->display_name, 0664,
> > > 					dev->clients_debug_root,
> > > 					client, &debug_client_fops);
> > >
> > > I find during I read the debugfs node, driver can do
> > > debugfs_remove_recursive(dentry); Is it expected?
> > 
> > Yes.  Well, not "expected", but a mess, yes.
> > 
> > Removing debugfs files are known to have lots of races, this isn't the only
> > one :(
> Thanks for the reply! 
> Not sure if there is any plan to resolve such races in the future?

Yes, I have "plans", but it's on my very long todo list behind lots of
other things...

If you want to look into it, please, that would be wonderful.

thanks,

greg k-h

RE: A race condition between debugfs and seq_file operation

[Lisa Du]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2111 bytes --]



> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: 2015年6月10日 13:20
> To: Lisa Du
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: A race condition between debugfs and seq_file operation
> 
> On Wed, Jun 10, 2015 at 05:00:03AM +0000, Lisa Du wrote:
> > > -----Original Message-----
> > > From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> > > Sent: 2015年6月10日 5:12
> > > To: Lisa Du
> > > Cc: linux-kernel@vger.kernel.org
> > > Subject: Re: A race condition between debugfs and seq_file operation
> > >
> > > On Mon, Jun 08, 2015 at 04:28:10AM +0000, Lisa Du wrote:
> > > > Hi, All
> > > > Recently I met one race condition related to debugfs.
> > > >
> > > > Take an example from ion.c in kernel3.14:
> > > > static int ion_debug_client_open(struct inode *inode, struct file
> > > > *file) {
> > > > 	return single_open(file, ion_debug_client_show,
> > > > inode->i_private); }
> > > >
> > > > static const struct file_operations debug_client_fops = {
> > > > 	.open = ion_debug_client_open,
> > > > 	.read = seq_read,
> > > > 	.llseek = seq_lseek,
> > > > 	.release = single_release,
> > > > };
> > > > client->debug_root = debugfs_create_file(client->display_name,
> > > > client->0664,
> > > > 					dev->clients_debug_root,
> > > > 					client, &debug_client_fops);
> > > >
> > > > I find during I read the debugfs node, driver can do
> > > > debugfs_remove_recursive(dentry); Is it expected?
> > >
> > > Yes.  Well, not "expected", but a mess, yes.
> > >
> > > Removing debugfs files are known to have lots of races, this isn't
> > > the only one :(
> > Thanks for the reply!
> > Not sure if there is any plan to resolve such races in the future?
> 
> Yes, I have "plans", but it's on my very long todo list behind lots of
> other things...
> 
> If you want to look into it, please, that would be wonderful.
Ok, I see, thanks!
> 
> thanks,
> 
> greg k-h
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: A race condition between debugfs and seq_file operation

[Matthias Schiffer]

[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]

On 06/10/2015 07:20 AM, gregkh@linuxfoundation.org wrote:
> On Wed, Jun 10, 2015 at 05:00:03AM +0000, Lisa Du wrote:
>>> -----Original Message-----
>>> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
>>> Sent: 2015年6月10日 5:12
>>> To: Lisa Du
>>> Cc: linux-kernel@vger.kernel.org
>>> Subject: Re: A race condition between debugfs and seq_file operation
>>>
>>> On Mon, Jun 08, 2015 at 04:28:10AM +0000, Lisa Du wrote:
>>>> Hi, All
>>>> Recently I met one race condition related to debugfs.
>>>>
>>>> Take an example from ion.c in kernel3.14:
>>>> static int ion_debug_client_open(struct inode *inode, struct file
>>>> *file) {
>>>> 	return single_open(file, ion_debug_client_show, inode->i_private); }
>>>>
>>>> static const struct file_operations debug_client_fops = {
>>>> 	.open = ion_debug_client_open,
>>>> 	.read = seq_read,
>>>> 	.llseek = seq_lseek,
>>>> 	.release = single_release,
>>>> };
>>>> client->debug_root = debugfs_create_file(client->display_name, 0664,
>>>> 					dev->clients_debug_root,
>>>> 					client, &debug_client_fops);
>>>>
>>>> I find during I read the debugfs node, driver can do
>>>> debugfs_remove_recursive(dentry); Is it expected?
>>>
>>> Yes.  Well, not "expected", but a mess, yes.
>>>
>>> Removing debugfs files are known to have lots of races, this isn't the only
>>> one :(
>> Thanks for the reply! 
>> Not sure if there is any plan to resolve such races in the future?
> 
> Yes, I have "plans", but it's on my very long todo list behind lots of
> other things...
> 
> If you want to look into it, please, that would be wonderful.
> 
> thanks,
> 
> greg k-h

I've stumbled across related issues a few days ago (mostly in network
drivers). What I've found out:

* I couldn't find any driver using device-specific debugfs files
removing them in a race-free way
* Userspace can make the race window arbitrarily large by opening a
debugfs file and reading from it later:

  modprobe batman-adv
  modprobe dummy
  echo bat0 > /sys/class/net/dummy0/batman_adv/mesh_iface
  (sleep 5; cat) < /sys/kernel/debug/batman_adv/bat0/originators &
  echo none > /sys/class/net/dummy0/batman_adv/mesh_iface
  # When the sleep finishs, batman-adv will read from a freed net_device

* There also seems to be a bug debugfs_remove_recursive hanging when
removing subdirectories with files that are still open:

  modprobe mac80211_hwsim
  # Or whatever phyX the hwsim PHY is
  (sleep 5; cat) < \
  /sys/kernel/debug/ieee80211/phy0/statistics/retry_count &
  rmmod mac80211_hwsim
  # Will hang in wiphy_unregister() until the sleep finishes,
  # with RTNL held!

Is there a sane way to check from the read fops callback if the file has
been removed (and lock against removal while doing that)? The nice
debugfs_create_u32() etc. helpers are useless as well for dynamic files
at the moment as they can't be used without this race condition...

I'd also like to get this cleaned up as soon as possible as changes I
plan for batman-adv might make the issue more prominent there.

Matthias


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: A race condition between debugfs and seq_file operation

[gregkh]

On Wed, Jun 10, 2015 at 04:00:37PM +0200, Matthias Schiffer wrote:
> On 06/10/2015 07:20 AM, gregkh@linuxfoundation.org wrote:
> > On Wed, Jun 10, 2015 at 05:00:03AM +0000, Lisa Du wrote:
> >>> -----Original Message-----
> >>> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> >>> Sent: 2015年6月10日 5:12
> >>> To: Lisa Du
> >>> Cc: linux-kernel@vger.kernel.org
> >>> Subject: Re: A race condition between debugfs and seq_file operation
> >>>
> >>> On Mon, Jun 08, 2015 at 04:28:10AM +0000, Lisa Du wrote:
> >>>> Hi, All
> >>>> Recently I met one race condition related to debugfs.
> >>>>
> >>>> Take an example from ion.c in kernel3.14:
> >>>> static int ion_debug_client_open(struct inode *inode, struct file
> >>>> *file) {
> >>>> 	return single_open(file, ion_debug_client_show, inode->i_private); }
> >>>>
> >>>> static const struct file_operations debug_client_fops = {
> >>>> 	.open = ion_debug_client_open,
> >>>> 	.read = seq_read,
> >>>> 	.llseek = seq_lseek,
> >>>> 	.release = single_release,
> >>>> };
> >>>> client->debug_root = debugfs_create_file(client->display_name, 0664,
> >>>> 					dev->clients_debug_root,
> >>>> 					client, &debug_client_fops);
> >>>>
> >>>> I find during I read the debugfs node, driver can do
> >>>> debugfs_remove_recursive(dentry); Is it expected?
> >>>
> >>> Yes.  Well, not "expected", but a mess, yes.
> >>>
> >>> Removing debugfs files are known to have lots of races, this isn't the only
> >>> one :(
> >> Thanks for the reply! 
> >> Not sure if there is any plan to resolve such races in the future?
> > 
> > Yes, I have "plans", but it's on my very long todo list behind lots of
> > other things...
> > 
> > If you want to look into it, please, that would be wonderful.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I've stumbled across related issues a few days ago (mostly in network
> drivers). What I've found out:

<snip>

Yes, all of these are issues, as I mentioned.

greg k-h