From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 6BF03209F26 for ; Fri, 10 Jan 2025 09:22:03 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736500925; cv=none; b=YCrt2+aILdLJRTnbB45sih5+cavWVlUZhs5YErYeuWhy5ZeRf8RS74tyMAmetQ/R1x8cVot2rd1/3CJRWbpYgv+24/jhzWDHkfH+l7K0v9Q4VC+FvbpwrFXpEIYEH30EvICYZl1ezP/U9I+AZKb+OEXKysySxUGnrpkYGDKkI9Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736500925; c=relaxed/simple; bh=lfFaMZ9TTPZwGWMiuTCajhU5dD4AfFP+5BtJ6JECZV4=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Wi5iAyvyhMMj2TZD/n4HxaujBSmP1IlJoE7zRdVLKhbMSgrpFS6yyt006uy1nMhEZzdJeL94pRkFCE/rL9FIoOH9h9656gvwZG4z0pfltkbXpiNTYHIRGLOC0NUozgCSZxbPXvr/dX5kHVcs8tY3L4+dpROsioqzhayFslTOglo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id ED48512FC; Fri, 10 Jan 2025 01:22:30 -0800 (PST) Received: from [10.57.94.123] (unknown [10.57.94.123]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 664D33F673; Fri, 10 Jan 2025 01:22:01 -0800 (PST) Message-ID: <86ebd184-1f65-488a-8d5a-5d203be70097@arm.com> Date: Fri, 10 Jan 2025 09:21:59 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] fs: debugfs: differentiate short fops with proxy ops Content-Language: en-GB To: Johannes Berg , linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , "Rafael J. Wysocki" , Johannes Berg , Geert Uytterhoeven , Aishwarya TCV , Mark Brown References: <20241129121536.30989-2-johannes@sipsolutions.net> From: Ryan Roberts In-Reply-To: <20241129121536.30989-2-johannes@sipsolutions.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Johannes, On 29/11/2024 12:15, Johannes Berg wrote: > From: Johannes Berg > > 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 > 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 > --- > 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)