diff mbox series

[v2,08/10] blktrace: add checks for created debugfs files on setup

Message ID 20200419194529.4872-9-mcgrof@kernel.org (mailing list archive)
State New, archived
Headers show
Series block: fix blktrace debugfs use after free | expand

Commit Message

Luis Chamberlain April 19, 2020, 7:45 p.m. UTC
Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
select DEBUG_FS, and blktrace exposes an API which userspace uses
relying on certain files created in debugfs. If files are not created
blktrace will not work correctly, so we do want to ensure that a
blktrace setup creates these files properly, and otherwise inform
userspace.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 kernel/trace/blktrace.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Bart Van Assche April 19, 2020, 10:57 p.m. UTC | #1
On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
> select DEBUG_FS, and blktrace exposes an API which userspace uses
> relying on certain files created in debugfs. If files are not created
> blktrace will not work correctly, so we do want to ensure that a
> blktrace setup creates these files properly, and otherwise inform
> userspace.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>   kernel/trace/blktrace.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 9cc0153849c3..fc32a8665ce8 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
>   					  struct dentry *dir,
>   					  struct blk_trace *bt)
>   {
> -	int ret = -EIO;
> -
>   	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
>   					       &blk_dropped_fops);
> +	if (!bt->dropped_file)
> +		return -ENOMEM;
>   
>   	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
> +	if (!bt->msg_file)
> +		return -ENOMEM;
>   
>   	bt->rchan = relay_open("trace", dir, buts->buf_size,
>   				buts->buf_nr, &blk_relay_callbacks, bt);
>   	if (!bt->rchan)
> -		return ret;
> +		return -EIO;
>   
>   	return 0;
>   }

I should have had a look at this patch before I replied to the previous 
patch.

Do you agree that the following code can be triggered by 
debugfs_create_file() and also that debugfs_create_file() never returns 
NULL?

static struct dentry *failed_creating(struct dentry *dentry)
{
	inode_unlock(d_inode(dentry->d_parent));
	dput(dentry);
	simple_release_fs(&debugfs_mount, &debugfs_mount_count);
	return ERR_PTR(-ENOMEM);
}

Thanks,

Bart.
Luis Chamberlain April 19, 2020, 11:05 p.m. UTC | #2
On Sun, Apr 19, 2020 at 03:57:58PM -0700, Bart Van Assche wrote:
> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
> > select DEBUG_FS, and blktrace exposes an API which userspace uses
> > relying on certain files created in debugfs. If files are not created
> > blktrace will not work correctly, so we do want to ensure that a
> > blktrace setup creates these files properly, and otherwise inform
> > userspace.
> > 
> > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > ---
> >   kernel/trace/blktrace.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > index 9cc0153849c3..fc32a8665ce8 100644
> > --- a/kernel/trace/blktrace.c
> > +++ b/kernel/trace/blktrace.c
> > @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
> >   					  struct dentry *dir,
> >   					  struct blk_trace *bt)
> >   {
> > -	int ret = -EIO;
> > -
> >   	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
> >   					       &blk_dropped_fops);
> > +	if (!bt->dropped_file)
> > +		return -ENOMEM;
> >   	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
> > +	if (!bt->msg_file)
> > +		return -ENOMEM;
> >   	bt->rchan = relay_open("trace", dir, buts->buf_size,
> >   				buts->buf_nr, &blk_relay_callbacks, bt);
> >   	if (!bt->rchan)
> > -		return ret;
> > +		return -EIO;
> >   	return 0;
> >   }
> 
> I should have had a look at this patch before I replied to the previous
> patch.
> 
> Do you agree that the following code can be triggered by
> debugfs_create_file() and also that debugfs_create_file() never returns
> NULL?

If debugfs is enabled, and not that we know it is in this blktrace code,
as we select it, it can return ERR_PTR(-ERROR) if an error occurs.

  Luis
Bart Van Assche April 19, 2020, 11:17 p.m. UTC | #3
On 4/19/20 4:05 PM, Luis Chamberlain wrote:
> On Sun, Apr 19, 2020 at 03:57:58PM -0700, Bart Van Assche wrote:
>> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
>>> Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
>>> select DEBUG_FS, and blktrace exposes an API which userspace uses
>>> relying on certain files created in debugfs. If files are not created
>>> blktrace will not work correctly, so we do want to ensure that a
>>> blktrace setup creates these files properly, and otherwise inform
>>> userspace.
>>>
>>> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
>>> ---
>>>    kernel/trace/blktrace.c | 8 +++++---
>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
>>> index 9cc0153849c3..fc32a8665ce8 100644
>>> --- a/kernel/trace/blktrace.c
>>> +++ b/kernel/trace/blktrace.c
>>> @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
>>>    					  struct dentry *dir,
>>>    					  struct blk_trace *bt)
>>>    {
>>> -	int ret = -EIO;
>>> -
>>>    	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
>>>    					       &blk_dropped_fops);
>>> +	if (!bt->dropped_file)
>>> +		return -ENOMEM;
>>>    	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
>>> +	if (!bt->msg_file)
>>> +		return -ENOMEM;
>>>    	bt->rchan = relay_open("trace", dir, buts->buf_size,
>>>    				buts->buf_nr, &blk_relay_callbacks, bt);
>>>    	if (!bt->rchan)
>>> -		return ret;
>>> +		return -EIO;
>>>    	return 0;
>>>    }
>>
>> I should have had a look at this patch before I replied to the previous
>> patch.
>>
>> Do you agree that the following code can be triggered by
>> debugfs_create_file() and also that debugfs_create_file() never returns
>> NULL?
> 
> If debugfs is enabled, and not that we know it is in this blktrace code,
> as we select it, it can return ERR_PTR(-ERROR) if an error occurs.

This is what I found in include/linux/debugfs.h in case debugfs is disabled:

static inline struct dentry *debugfs_create_file(const char *name,
	umode_t mode, struct dentry *parent, void *data,
	const struct file_operations *fops)
{
	return ERR_PTR(-ENODEV);
}

I have not found any code path that can cause debugfs_create_file() to 
return NULL. Did I perhaps overlook something? If not, it's not clear to 
me why the above patch adds checks that check whether 
debugfs_create_file() returns NULL?

Thanks,

Bart.
Greg KH April 20, 2020, 11:39 a.m. UTC | #4
On Sun, Apr 19, 2020 at 07:45:27PM +0000, Luis Chamberlain wrote:
> Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
> select DEBUG_FS, and blktrace exposes an API which userspace uses
> relying on certain files created in debugfs. If files are not created
> blktrace will not work correctly, so we do want to ensure that a
> blktrace setup creates these files properly, and otherwise inform
> userspace.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  kernel/trace/blktrace.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 9cc0153849c3..fc32a8665ce8 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
>  					  struct dentry *dir,
>  					  struct blk_trace *bt)
>  {
> -	int ret = -EIO;
> -
>  	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
>  					       &blk_dropped_fops);
> +	if (!bt->dropped_file)
> +		return -ENOMEM;

No, this is wrong, please do not ever check the return value of a
debugfs call.  See the zillions of patches I've been doing to the kernel
for this type of thing over the past year for examples of why.

the code is fine as-is.

greg k-h
Greg KH April 20, 2020, 11:40 a.m. UTC | #5
On Sun, Apr 19, 2020 at 04:17:46PM -0700, Bart Van Assche wrote:
> On 4/19/20 4:05 PM, Luis Chamberlain wrote:
> > On Sun, Apr 19, 2020 at 03:57:58PM -0700, Bart Van Assche wrote:
> > > On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > > > Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
> > > > select DEBUG_FS, and blktrace exposes an API which userspace uses
> > > > relying on certain files created in debugfs. If files are not created
> > > > blktrace will not work correctly, so we do want to ensure that a
> > > > blktrace setup creates these files properly, and otherwise inform
> > > > userspace.
> > > > 
> > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > ---
> > > >    kernel/trace/blktrace.c | 8 +++++---
> > > >    1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > > > index 9cc0153849c3..fc32a8665ce8 100644
> > > > --- a/kernel/trace/blktrace.c
> > > > +++ b/kernel/trace/blktrace.c
> > > > @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
> > > >    					  struct dentry *dir,
> > > >    					  struct blk_trace *bt)
> > > >    {
> > > > -	int ret = -EIO;
> > > > -
> > > >    	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
> > > >    					       &blk_dropped_fops);
> > > > +	if (!bt->dropped_file)
> > > > +		return -ENOMEM;
> > > >    	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
> > > > +	if (!bt->msg_file)
> > > > +		return -ENOMEM;
> > > >    	bt->rchan = relay_open("trace", dir, buts->buf_size,
> > > >    				buts->buf_nr, &blk_relay_callbacks, bt);
> > > >    	if (!bt->rchan)
> > > > -		return ret;
> > > > +		return -EIO;
> > > >    	return 0;
> > > >    }
> > > 
> > > I should have had a look at this patch before I replied to the previous
> > > patch.
> > > 
> > > Do you agree that the following code can be triggered by
> > > debugfs_create_file() and also that debugfs_create_file() never returns
> > > NULL?
> > 
> > If debugfs is enabled, and not that we know it is in this blktrace code,
> > as we select it, it can return ERR_PTR(-ERROR) if an error occurs.
> 
> This is what I found in include/linux/debugfs.h in case debugfs is disabled:
> 
> static inline struct dentry *debugfs_create_file(const char *name,
> 	umode_t mode, struct dentry *parent, void *data,
> 	const struct file_operations *fops)
> {
> 	return ERR_PTR(-ENODEV);
> }
> 
> I have not found any code path that can cause debugfs_create_file() to
> return NULL. Did I perhaps overlook something? If not, it's not clear to me
> why the above patch adds checks that check whether debugfs_create_file()
> returns NULL?

Short answer, yes, it can return NULL.  Correct answer is, you don't
care, don't check the value and don't do anything about it.  It's
debugging code, userspace doesn't care, so just keep moving on.

thanks,

greg k-h
Luis Chamberlain April 20, 2020, 6:44 p.m. UTC | #6
On Mon, Apr 20, 2020 at 01:40:38PM +0200, Greg KH wrote:
> On Sun, Apr 19, 2020 at 04:17:46PM -0700, Bart Van Assche wrote:
> > On 4/19/20 4:05 PM, Luis Chamberlain wrote:
> > > On Sun, Apr 19, 2020 at 03:57:58PM -0700, Bart Van Assche wrote:
> > > > On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > > > > Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
> > > > > select DEBUG_FS, and blktrace exposes an API which userspace uses
> > > > > relying on certain files created in debugfs. If files are not created
> > > > > blktrace will not work correctly, so we do want to ensure that a
> > > > > blktrace setup creates these files properly, and otherwise inform
> > > > > userspace.
> > > > > 
> > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > > ---
> > > > >    kernel/trace/blktrace.c | 8 +++++---
> > > > >    1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > > > > index 9cc0153849c3..fc32a8665ce8 100644
> > > > > --- a/kernel/trace/blktrace.c
> > > > > +++ b/kernel/trace/blktrace.c
> > > > > @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
> > > > >    					  struct dentry *dir,
> > > > >    					  struct blk_trace *bt)
> > > > >    {
> > > > > -	int ret = -EIO;
> > > > > -
> > > > >    	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
> > > > >    					       &blk_dropped_fops);
> > > > > +	if (!bt->dropped_file)
> > > > > +		return -ENOMEM;
> > > > >    	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
> > > > > +	if (!bt->msg_file)
> > > > > +		return -ENOMEM;
> > > > >    	bt->rchan = relay_open("trace", dir, buts->buf_size,
> > > > >    				buts->buf_nr, &blk_relay_callbacks, bt);
> > > > >    	if (!bt->rchan)
> > > > > -		return ret;
> > > > > +		return -EIO;
> > > > >    	return 0;
> > > > >    }
> > > > 
> > > > I should have had a look at this patch before I replied to the previous
> > > > patch.
> > > > 
> > > > Do you agree that the following code can be triggered by
> > > > debugfs_create_file() and also that debugfs_create_file() never returns
> > > > NULL?
> > > 
> > > If debugfs is enabled, and not that we know it is in this blktrace code,
> > > as we select it, it can return ERR_PTR(-ERROR) if an error occurs.
> > 
> > This is what I found in include/linux/debugfs.h in case debugfs is disabled:
> > 
> > static inline struct dentry *debugfs_create_file(const char *name,
> > 	umode_t mode, struct dentry *parent, void *data,
> > 	const struct file_operations *fops)
> > {
> > 	return ERR_PTR(-ENODEV);
> > }
> > 
> > I have not found any code path that can cause debugfs_create_file() to
> > return NULL. Did I perhaps overlook something? If not, it's not clear to me
> > why the above patch adds checks that check whether debugfs_create_file()
> > returns NULL?
> 
> Short answer, yes, it can return NULL.  Correct answer is, you don't
> care, don't check the value and don't do anything about it.  It's
> debugging code, userspace doesn't care, so just keep moving on.

Thing is this code *exposes* knobs to userspace for an API that *does*
exepect those files to exist. That is, blktrace *relies* on these
debugfs files to exist. So the kconfig which enables blktrace
CONFIG_BLK_DEV_IO_TRACE selects DEBUG_FS.

So typically we don't care if these files were created or not on regular
drivers, but in this case this code is only compiled when debugfs is
enabled and CONFIG_BLK_DEV_IO_TRACE, and the userspace interaction with
debugfs *expects* these files.

So what do you recommend?

  Luis
Greg KH April 20, 2020, 8:11 p.m. UTC | #7
On Mon, Apr 20, 2020 at 06:44:45PM +0000, Luis Chamberlain wrote:
> On Mon, Apr 20, 2020 at 01:40:38PM +0200, Greg KH wrote:
> > On Sun, Apr 19, 2020 at 04:17:46PM -0700, Bart Van Assche wrote:
> > > On 4/19/20 4:05 PM, Luis Chamberlain wrote:
> > > > On Sun, Apr 19, 2020 at 03:57:58PM -0700, Bart Van Assche wrote:
> > > > > On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > > > > > Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
> > > > > > select DEBUG_FS, and blktrace exposes an API which userspace uses
> > > > > > relying on certain files created in debugfs. If files are not created
> > > > > > blktrace will not work correctly, so we do want to ensure that a
> > > > > > blktrace setup creates these files properly, and otherwise inform
> > > > > > userspace.
> > > > > > 
> > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > > > ---
> > > > > >    kernel/trace/blktrace.c | 8 +++++---
> > > > > >    1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > > > > > index 9cc0153849c3..fc32a8665ce8 100644
> > > > > > --- a/kernel/trace/blktrace.c
> > > > > > +++ b/kernel/trace/blktrace.c
> > > > > > @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
> > > > > >    					  struct dentry *dir,
> > > > > >    					  struct blk_trace *bt)
> > > > > >    {
> > > > > > -	int ret = -EIO;
> > > > > > -
> > > > > >    	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
> > > > > >    					       &blk_dropped_fops);
> > > > > > +	if (!bt->dropped_file)
> > > > > > +		return -ENOMEM;
> > > > > >    	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
> > > > > > +	if (!bt->msg_file)
> > > > > > +		return -ENOMEM;
> > > > > >    	bt->rchan = relay_open("trace", dir, buts->buf_size,
> > > > > >    				buts->buf_nr, &blk_relay_callbacks, bt);
> > > > > >    	if (!bt->rchan)
> > > > > > -		return ret;
> > > > > > +		return -EIO;
> > > > > >    	return 0;
> > > > > >    }
> > > > > 
> > > > > I should have had a look at this patch before I replied to the previous
> > > > > patch.
> > > > > 
> > > > > Do you agree that the following code can be triggered by
> > > > > debugfs_create_file() and also that debugfs_create_file() never returns
> > > > > NULL?
> > > > 
> > > > If debugfs is enabled, and not that we know it is in this blktrace code,
> > > > as we select it, it can return ERR_PTR(-ERROR) if an error occurs.
> > > 
> > > This is what I found in include/linux/debugfs.h in case debugfs is disabled:
> > > 
> > > static inline struct dentry *debugfs_create_file(const char *name,
> > > 	umode_t mode, struct dentry *parent, void *data,
> > > 	const struct file_operations *fops)
> > > {
> > > 	return ERR_PTR(-ENODEV);
> > > }
> > > 
> > > I have not found any code path that can cause debugfs_create_file() to
> > > return NULL. Did I perhaps overlook something? If not, it's not clear to me
> > > why the above patch adds checks that check whether debugfs_create_file()
> > > returns NULL?
> > 
> > Short answer, yes, it can return NULL.  Correct answer is, you don't
> > care, don't check the value and don't do anything about it.  It's
> > debugging code, userspace doesn't care, so just keep moving on.
> 
> Thing is this code *exposes* knobs to userspace for an API that *does*
> exepect those files to exist. That is, blktrace *relies* on these
> debugfs files to exist. So the kconfig which enables blktrace
> CONFIG_BLK_DEV_IO_TRACE selects DEBUG_FS.

That's nice, but again, no kernel code should do anything different
depending on what debugfs happens to be doing at that point in time.

> So typically we don't care if these files were created or not on regular
> drivers, but in this case this code is only compiled when debugfs is
> enabled and CONFIG_BLK_DEV_IO_TRACE, and the userspace interaction with
> debugfs *expects* these files.
> 
> So what do you recommend?

Make sure that userspace can handle the files not being there and keep
on working properly if they aren't.

As you can't "recover" from debugfs failing, there's no need to check
anything with it.

thanks,

greg k-h
Luis Chamberlain April 20, 2020, 8:20 p.m. UTC | #8
On Mon, Apr 20, 2020 at 10:11:01PM +0200, Greg KH wrote:
> On Mon, Apr 20, 2020 at 06:44:45PM +0000, Luis Chamberlain wrote:
> > On Mon, Apr 20, 2020 at 01:40:38PM +0200, Greg KH wrote:
> > > On Sun, Apr 19, 2020 at 04:17:46PM -0700, Bart Van Assche wrote:
> > > > On 4/19/20 4:05 PM, Luis Chamberlain wrote:
> > > > > On Sun, Apr 19, 2020 at 03:57:58PM -0700, Bart Van Assche wrote:
> > > > > > On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > > > > > > Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
> > > > > > > select DEBUG_FS, and blktrace exposes an API which userspace uses
> > > > > > > relying on certain files created in debugfs. If files are not created
> > > > > > > blktrace will not work correctly, so we do want to ensure that a
> > > > > > > blktrace setup creates these files properly, and otherwise inform
> > > > > > > userspace.
> > > > > > > 
> > > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > > > > ---
> > > > > > >    kernel/trace/blktrace.c | 8 +++++---
> > > > > > >    1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > > > > > > index 9cc0153849c3..fc32a8665ce8 100644
> > > > > > > --- a/kernel/trace/blktrace.c
> > > > > > > +++ b/kernel/trace/blktrace.c
> > > > > > > @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
> > > > > > >    					  struct dentry *dir,
> > > > > > >    					  struct blk_trace *bt)
> > > > > > >    {
> > > > > > > -	int ret = -EIO;
> > > > > > > -
> > > > > > >    	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
> > > > > > >    					       &blk_dropped_fops);
> > > > > > > +	if (!bt->dropped_file)
> > > > > > > +		return -ENOMEM;
> > > > > > >    	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
> > > > > > > +	if (!bt->msg_file)
> > > > > > > +		return -ENOMEM;
> > > > > > >    	bt->rchan = relay_open("trace", dir, buts->buf_size,
> > > > > > >    				buts->buf_nr, &blk_relay_callbacks, bt);
> > > > > > >    	if (!bt->rchan)
> > > > > > > -		return ret;
> > > > > > > +		return -EIO;
> > > > > > >    	return 0;
> > > > > > >    }
> > > > > > 
> > > > > > I should have had a look at this patch before I replied to the previous
> > > > > > patch.
> > > > > > 
> > > > > > Do you agree that the following code can be triggered by
> > > > > > debugfs_create_file() and also that debugfs_create_file() never returns
> > > > > > NULL?
> > > > > 
> > > > > If debugfs is enabled, and not that we know it is in this blktrace code,
> > > > > as we select it, it can return ERR_PTR(-ERROR) if an error occurs.
> > > > 
> > > > This is what I found in include/linux/debugfs.h in case debugfs is disabled:
> > > > 
> > > > static inline struct dentry *debugfs_create_file(const char *name,
> > > > 	umode_t mode, struct dentry *parent, void *data,
> > > > 	const struct file_operations *fops)
> > > > {
> > > > 	return ERR_PTR(-ENODEV);
> > > > }
> > > > 
> > > > I have not found any code path that can cause debugfs_create_file() to
> > > > return NULL. Did I perhaps overlook something? If not, it's not clear to me
> > > > why the above patch adds checks that check whether debugfs_create_file()
> > > > returns NULL?
> > > 
> > > Short answer, yes, it can return NULL.  Correct answer is, you don't
> > > care, don't check the value and don't do anything about it.  It's
> > > debugging code, userspace doesn't care, so just keep moving on.
> > 
> > Thing is this code *exposes* knobs to userspace for an API that *does*
> > exepect those files to exist. That is, blktrace *relies* on these
> > debugfs files to exist. So the kconfig which enables blktrace
> > CONFIG_BLK_DEV_IO_TRACE selects DEBUG_FS.
> 
> That's nice, but again, no kernel code should do anything different
> depending on what debugfs happens to be doing at that point in time.

So even if the debugfs files were *not* created, and this code executes only
if DEBUG_FS, you don't think we should inform userspace if the blktrace
setup ioctl, which sets up these debugfs, didn't happen?

The "recovery" here would just be to destroy the blktrace setup, and
inform userspace that the blktrace setup ioctl failed.

  Luis
Greg KH April 21, 2020, 6:55 a.m. UTC | #9
On Mon, Apr 20, 2020 at 08:20:46PM +0000, Luis Chamberlain wrote:
> On Mon, Apr 20, 2020 at 10:11:01PM +0200, Greg KH wrote:
> > On Mon, Apr 20, 2020 at 06:44:45PM +0000, Luis Chamberlain wrote:
> > > On Mon, Apr 20, 2020 at 01:40:38PM +0200, Greg KH wrote:
> > > > On Sun, Apr 19, 2020 at 04:17:46PM -0700, Bart Van Assche wrote:
> > > > > On 4/19/20 4:05 PM, Luis Chamberlain wrote:
> > > > > > On Sun, Apr 19, 2020 at 03:57:58PM -0700, Bart Van Assche wrote:
> > > > > > > On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > > > > > > > Even though debugfs can be disabled, enabling BLK_DEV_IO_TRACE will
> > > > > > > > select DEBUG_FS, and blktrace exposes an API which userspace uses
> > > > > > > > relying on certain files created in debugfs. If files are not created
> > > > > > > > blktrace will not work correctly, so we do want to ensure that a
> > > > > > > > blktrace setup creates these files properly, and otherwise inform
> > > > > > > > userspace.
> > > > > > > > 
> > > > > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> > > > > > > > ---
> > > > > > > >    kernel/trace/blktrace.c | 8 +++++---
> > > > > > > >    1 file changed, 5 insertions(+), 3 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> > > > > > > > index 9cc0153849c3..fc32a8665ce8 100644
> > > > > > > > --- a/kernel/trace/blktrace.c
> > > > > > > > +++ b/kernel/trace/blktrace.c
> > > > > > > > @@ -552,17 +552,19 @@ static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
> > > > > > > >    					  struct dentry *dir,
> > > > > > > >    					  struct blk_trace *bt)
> > > > > > > >    {
> > > > > > > > -	int ret = -EIO;
> > > > > > > > -
> > > > > > > >    	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
> > > > > > > >    					       &blk_dropped_fops);
> > > > > > > > +	if (!bt->dropped_file)
> > > > > > > > +		return -ENOMEM;
> > > > > > > >    	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
> > > > > > > > +	if (!bt->msg_file)
> > > > > > > > +		return -ENOMEM;
> > > > > > > >    	bt->rchan = relay_open("trace", dir, buts->buf_size,
> > > > > > > >    				buts->buf_nr, &blk_relay_callbacks, bt);
> > > > > > > >    	if (!bt->rchan)
> > > > > > > > -		return ret;
> > > > > > > > +		return -EIO;
> > > > > > > >    	return 0;
> > > > > > > >    }
> > > > > > > 
> > > > > > > I should have had a look at this patch before I replied to the previous
> > > > > > > patch.
> > > > > > > 
> > > > > > > Do you agree that the following code can be triggered by
> > > > > > > debugfs_create_file() and also that debugfs_create_file() never returns
> > > > > > > NULL?
> > > > > > 
> > > > > > If debugfs is enabled, and not that we know it is in this blktrace code,
> > > > > > as we select it, it can return ERR_PTR(-ERROR) if an error occurs.
> > > > > 
> > > > > This is what I found in include/linux/debugfs.h in case debugfs is disabled:
> > > > > 
> > > > > static inline struct dentry *debugfs_create_file(const char *name,
> > > > > 	umode_t mode, struct dentry *parent, void *data,
> > > > > 	const struct file_operations *fops)
> > > > > {
> > > > > 	return ERR_PTR(-ENODEV);
> > > > > }
> > > > > 
> > > > > I have not found any code path that can cause debugfs_create_file() to
> > > > > return NULL. Did I perhaps overlook something? If not, it's not clear to me
> > > > > why the above patch adds checks that check whether debugfs_create_file()
> > > > > returns NULL?
> > > > 
> > > > Short answer, yes, it can return NULL.  Correct answer is, you don't
> > > > care, don't check the value and don't do anything about it.  It's
> > > > debugging code, userspace doesn't care, so just keep moving on.
> > > 
> > > Thing is this code *exposes* knobs to userspace for an API that *does*
> > > exepect those files to exist. That is, blktrace *relies* on these
> > > debugfs files to exist. So the kconfig which enables blktrace
> > > CONFIG_BLK_DEV_IO_TRACE selects DEBUG_FS.
> > 
> > That's nice, but again, no kernel code should do anything different
> > depending on what debugfs happens to be doing at that point in time.
> 
> So even if the debugfs files were *not* created, and this code executes only
> if DEBUG_FS, you don't think we should inform userspace if the blktrace
> setup ioctl, which sets up these debugfs, didn't happen?
> 
> The "recovery" here would just be to destroy the blktrace setup, and
> inform userspace that the blktrace setup ioctl failed.

Hm, ok, but comment the heck out of this saying _why_ you are testing
the return value, and how that differs from 99% of the other users of
this function in the kernel tree please.

Otherwise I will end up removing the checks again with my semi-regular
sweep of the tree...

thanks,

greg k-h
diff mbox series

Patch

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 9cc0153849c3..fc32a8665ce8 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -552,17 +552,19 @@  static int blk_trace_create_debugfs_files(struct blk_user_trace_setup *buts,
 					  struct dentry *dir,
 					  struct blk_trace *bt)
 {
-	int ret = -EIO;
-
 	bt->dropped_file = debugfs_create_file("dropped", 0444, dir, bt,
 					       &blk_dropped_fops);
+	if (!bt->dropped_file)
+		return -ENOMEM;
 
 	bt->msg_file = debugfs_create_file("msg", 0222, dir, bt, &blk_msg_fops);
+	if (!bt->msg_file)
+		return -ENOMEM;
 
 	bt->rchan = relay_open("trace", dir, buts->buf_size,
 				buts->buf_nr, &blk_relay_callbacks, bt);
 	if (!bt->rchan)
-		return ret;
+		return -EIO;
 
 	return 0;
 }