Message-ID: <86ebd184-1f65-488a-8d5a-5d203be70097@arm.com>
Date: Fri, 10 Jan 2025 09:21:59 +0000
From: Ryan Roberts <ryan.roberts@....com>
To: Johannes Berg <johannes@...solutions.net>, linux-kernel@...r.kernel.org
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Johannes Berg <johannes.berg@...el.com>,
 Geert Uytterhoeven <geert@...ux-m68k.org>,
 Aishwarya TCV <Aishwarya.TCV@....com>, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] fs: debugfs: differentiate short fops with proxy ops

Hi Johannes,

On 29/11/2024 12:15, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@...el.com>
> 
> Geert reported that my previous short fops debugfs changes
> broke m68k, because it only has mandatory alignement of two,
> so we can't stash the "is it short" information into the
> pointer (as we already did with the "is it real" bit.)
> 
> Instead, exploit the fact that debugfs_file_get() called on
> an already open file will already find that the fsdata is
> no longer the real fops but rather the allocated data that
> already distinguishes full/short ops, so only open() needs
> to be able to distinguish. We can achieve that by using two
> different open functions.
> 
> Unfortunately this requires another set of full file ops,
> increasing the size by 536 bytes (x86-64), but that's still
> a reasonable trade-off given that only converting some of
> the wireless stack gained over 28k. This brings the total
> cost of this to around 1k, for wins of 28k (all x86-64).
> 
> Reported-and-tested-by: Geert Uytterhoeven <geert@...ux-m68k.org>
> Link: https://lore.kernel.org/CAMuHMdWu_9-L2Te101w8hU7H_2yobJFPXSwwUmGHSJfaPWDKiQ@mail.gmail.com
> Fixes: 8dc6d81c6b2a ("debugfs: add small file operations for most files")
> Signed-off-by: Johannes Berg <johannes.berg@...el.com>
> ---
>  fs/debugfs/file.c     | 72 ++++++++++++++++++++++++++++++-------------
>  fs/debugfs/inode.c    | 11 +++----
>  fs/debugfs/internal.h |  6 +---
>  3 files changed, 55 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 47dc96dfe386..bdb4f2ca0506 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -64,22 +64,13 @@ const struct file_operations *debugfs_real_fops(const struct file *filp)
>  }
>  EXPORT_SYMBOL_GPL(debugfs_real_fops);
>  
> -/**
> - * debugfs_file_get - mark the beginning of file data access
> - * @dentry: the dentry object whose data is being accessed.
> - *
> - * Up to a matching call to debugfs_file_put(), any successive call
> - * into the file removing functions debugfs_remove() and
> - * debugfs_remove_recursive() will block. Since associated private
> - * file data may only get freed after a successful return of any of
> - * the removal functions, you may safely access it after a successful
> - * call to debugfs_file_get() without worrying about lifetime issues.
> - *
> - * If -%EIO is returned, the file has already been removed and thus,
> - * it is not safe to access any of its data. If, on the other hand,
> - * it is allowed to access the file data, zero is returned.
> - */
> -int debugfs_file_get(struct dentry *dentry)
> +enum dbgfs_get_mode {
> +	DBGFS_GET_ALREADY,
> +	DBGFS_GET_REGULAR,
> +	DBGFS_GET_SHORT,
> +};
> +
> +static int __debugfs_file_get(struct dentry *dentry, enum dbgfs_get_mode mode)
>  {
>  	struct debugfs_fsdata *fsd;
>  	void *d_fsd;
> @@ -96,15 +87,17 @@ int debugfs_file_get(struct dentry *dentry)
>  	if (!((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)) {
>  		fsd = d_fsd;
>  	} else {
> +		if (WARN_ON(mode == DBGFS_GET_ALREADY))
> +			return -EINVAL;

I'm seeing a problem on arm64 where opening /sys/kernel/debug/gup_test has 
started failing with EINVAL after this patch is applied. Additionally this 
warning is triggering (see below).

gup_test is a debugfs file created with CONFIG_GUP_TEST and used by the mm 
selftest (tools/testing/selftests/mm/gup_test.c). As a result, the tests are now 
getting skipped. Everything works as expected without this patch.

I guess you're expecting to always take the "fsd = d_fsd;" path for 
mode=DBGFS_GET_ALREADY, which we are not doing for some reason in this case?

---8<---
[   34.393031] ------------[ cut here ]------------
[   34.393036] WARNING: CPU: 2 PID: 1408 at fs/debugfs/file.c:90 __debugfs_file_get+0x1d0/0x1f8
[   34.393044] Modules linked in:
[   34.393047] CPU: 2 UID: 0 PID: 1408 Comm: gup_test Not tainted 6.13.0-rc3-00001-gf8f25893a477 #61
[   34.393049] Hardware name: linux,dummy-virt (DT)
[   34.393050] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   34.393052] pc : __debugfs_file_get+0x1d0/0x1f8
[   34.393055] lr : open_proxy_open+0x38/0x130
[   34.393057] sp : ffff80008d5b3ac0
[   34.393058] x29: ffff80008d5b3ac0 x28: ffff80008d5b3c78 x27: 0000000000000000
[   34.393061] x26: 0000000000000006 x25: ffff800083354cf8 x24: 0000000000000000
[   34.393064] x23: ffff000141390000 x22: 0000000000000000 x21: ffff800081670229
[   34.393067] x20: ffff00018b35ce40 x19: ffff0001411823c0 x18: 0000000000000000
[   34.393069] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
[   34.393071] x14: ffffffffffffffff x13: ffff0001424cd021 x12: ffff80008d5b3cbc
[   34.393074] x11: 00000003c3a7f0df x10: 00007ffeff8cd810 x9 : ffff80008071b258
[   34.393077] x8 : d0d0d0d0d0d0d0d0 x7 : 808080808080807f x6 : 0000000000000000
[   34.393079] x5 : 0000000000000064 x4 : ffff000141390158 x3 : 0000000000000001
[   34.393082] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 0000000000400000
[   34.393085] Call trace:
[   34.393086]  __debugfs_file_get+0x1d0/0x1f8 (P)
[   34.393089]  open_proxy_open+0x38/0x130
[   34.393092]  do_dentry_open+0x22c/0x690
[   34.393095]  vfs_open+0x34/0xf8
[   34.393097]  path_openat+0x2b8/0xe58
[   34.393101]  do_filp_open+0x94/0x158
[   34.393103]  do_sys_openat2+0xbc/0xf0
[   34.393106]  __arm64_sys_openat+0x68/0xb8
[   34.393108]  invoke_syscall+0x50/0x120
[   34.393111]  el0_svc_common.constprop.0+0x48/0xf8
[   34.393114]  do_el0_svc+0x28/0x40
[   34.393117]  el0_svc+0x30/0xd0
[   34.393120]  el0t_64_sync_handler+0x144/0x168
[   34.393122]  el0t_64_sync+0x198/0x1a0
[   34.393124] ---[ end trace 0000000000000000 ]---
---8<---

Thanks,
Ryan


> +
>  		fsd = kmalloc(sizeof(*fsd), GFP_KERNEL);
>  		if (!fsd)
>  			return -ENOMEM;
>  
> -		if ((unsigned long)d_fsd & DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT) {
> +		if (mode == DBGFS_GET_SHORT) {
>  			fsd->real_fops = NULL;
>  			fsd->short_fops = (void *)((unsigned long)d_fsd &
> -						~(DEBUGFS_FSDATA_IS_REAL_FOPS_BIT |
> -						  DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT));
> +						~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
>  		} else {
>  			fsd->real_fops = (void *)((unsigned long)d_fsd &
>  						~DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
> @@ -138,6 +131,26 @@ int debugfs_file_get(struct dentry *dentry)
>  
>  	return 0;
>  }
> +
> +/**
> + * debugfs_file_get - mark the beginning of file data access
> + * @dentry: the dentry object whose data is being accessed.
> + *
> + * Up to a matching call to debugfs_file_put(), any successive call
> + * into the file removing functions debugfs_remove() and
> + * debugfs_remove_recursive() will block. Since associated private
> + * file data may only get freed after a successful return of any of
> + * the removal functions, you may safely access it after a successful
> + * call to debugfs_file_get() without worrying about lifetime issues.
> + *
> + * If -%EIO is returned, the file has already been removed and thus,
> + * it is not safe to access any of its data. If, on the other hand,
> + * it is allowed to access the file data, zero is returned.
> + */
> +int debugfs_file_get(struct dentry *dentry)
> +{
> +	return __debugfs_file_get(dentry, DBGFS_GET_ALREADY);
> +}
>  EXPORT_SYMBOL_GPL(debugfs_file_get);
>  
>  /**
> @@ -424,7 +437,8 @@ static void __full_proxy_fops_init(struct file_operations *proxy_fops,
>  		proxy_fops->unlocked_ioctl = full_proxy_unlocked_ioctl;
>  }
>  
> -static int full_proxy_open(struct inode *inode, struct file *filp)
> +static int full_proxy_open(struct inode *inode, struct file *filp,
> +			   enum dbgfs_get_mode mode)
>  {
>  	struct dentry *dentry = F_DENTRY(filp);
>  	const struct file_operations *real_fops;
> @@ -432,7 +446,7 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
>  	struct debugfs_fsdata *fsd;
>  	int r;
>  
> -	r = debugfs_file_get(dentry);
> +	r = __debugfs_file_get(dentry, mode);
>  	if (r)
>  		return r == -EIO ? -ENOENT : r;
>  
> @@ -491,8 +505,22 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
>  	return r;
>  }
>  
> +static int full_proxy_open_regular(struct inode *inode, struct file *filp)
> +{
> +	return full_proxy_open(inode, filp, DBGFS_GET_REGULAR);
> +}
> +
>  const struct file_operations debugfs_full_proxy_file_operations = {
> -	.open = full_proxy_open,
> +	.open = full_proxy_open_regular,
> +};
> +
> +static int full_proxy_open_short(struct inode *inode, struct file *filp)
> +{
> +	return full_proxy_open(inode, filp, DBGFS_GET_SHORT);
> +}
> +
> +const struct file_operations debugfs_full_short_proxy_file_operations = {
> +	.open = full_proxy_open_short,
>  };
>  
>  ssize_t debugfs_attr_read(struct file *file, char __user *buf,
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 38a9c7eb97e6..65e46c7b6bf1 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -455,8 +455,7 @@ struct dentry *debugfs_create_file_full(const char *name, umode_t mode,
>  					const struct file_operations *fops)
>  {
>  	if (WARN_ON((unsigned long)fops &
> -		    (DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT |
> -		     DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)))
> +		    DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
>  		return ERR_PTR(-EINVAL);
>  
>  	return __debugfs_create_file(name, mode, parent, data,
> @@ -471,15 +470,13 @@ struct dentry *debugfs_create_file_short(const char *name, umode_t mode,
>  					 const struct debugfs_short_fops *fops)
>  {
>  	if (WARN_ON((unsigned long)fops &
> -		    (DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT |
> -		     DEBUGFS_FSDATA_IS_REAL_FOPS_BIT)))
> +		    DEBUGFS_FSDATA_IS_REAL_FOPS_BIT))
>  		return ERR_PTR(-EINVAL);
>  
>  	return __debugfs_create_file(name, mode, parent, data,
> -				fops ? &debugfs_full_proxy_file_operations :
> +				fops ? &debugfs_full_short_proxy_file_operations :
>  					&debugfs_noop_file_operations,
> -				(const void *)((unsigned long)fops |
> -					       DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT));
> +				fops);
>  }
>  EXPORT_SYMBOL_GPL(debugfs_create_file_short);
>  
> diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
> index a3edfa4f0d8e..bbae4a228ef4 100644
> --- a/fs/debugfs/internal.h
> +++ b/fs/debugfs/internal.h
> @@ -15,6 +15,7 @@ struct file_operations;
>  extern const struct file_operations debugfs_noop_file_operations;
>  extern const struct file_operations debugfs_open_proxy_file_operations;
>  extern const struct file_operations debugfs_full_proxy_file_operations;
> +extern const struct file_operations debugfs_full_short_proxy_file_operations;
>  
>  struct debugfs_fsdata {
>  	const struct file_operations *real_fops;
> @@ -40,11 +41,6 @@ struct debugfs_fsdata {
>   * pointer gets its lowest bit set.
>   */
>  #define DEBUGFS_FSDATA_IS_REAL_FOPS_BIT BIT(0)
> -/*
> - * A dentry's ->d_fsdata, when pointing to real fops, is with
> - * short fops instead of full fops.
> - */
> -#define DEBUGFS_FSDATA_IS_SHORT_FOPS_BIT BIT(1)
>  
>  /* Access BITS */
>  #define DEBUGFS_ALLOW_API	BIT(0)

Powered by blists - more mailing lists