| Message ID | 20210518163304.3702015-1-gregkh@linuxfoundation.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] b43: don't save dentries for debugfs | expand |
On Tue, May 18, 2021 at 03:00:44PM -0700, Jeff Johnson wrote: > On 2021-05-18 12:29, Jeff Johnson wrote: > > On 2021-05-18 09:33, Greg Kroah-Hartman wrote: > > > There is no need to keep around the dentry pointers for the debugfs > > > files as they will all be automatically removed when the subdir is > > > removed. So save the space and logic involved in keeping them > > > around by > > > just getting rid of them entirely. > > > > > > By doing this change, we remove one of the last in-kernel user that > > > was > > > storing the result of debugfs_create_bool(), so that api can be > > > cleaned > > > up. > > > > Question not about this specific change, but the general concept > > of keeping (or not keeping) dentry pointers. In the ath drivers, > > as well as in an out-of-tree driver for Android, we keep a > > debugfs dentry pointer to use as a param to relay_open(). > > > > Will we still be able to have a dentry pointer for this purpose? > > Or better, is there a recommended way to get a dentry pointer > > NOT associated with debugfs at all (which would be ideal for > > Android where debugfs is disabled). > > Answering one of my questions: The dentry passed to relay_open() comes > from debugfs_create_dir() which is expected to return a dentry. > > Would still like guidance on if there is a recommended way to get a > dentry not associated with debugfs. What do you exactly mean by "not associated with debugfs"? And why are you passing a debugfs dentry to relay_open()? That feels really wrong and fragile. Ideally I want to get rid of the "raw" dentry that debugfs returns to callers, as it has caused odd problems in the past, but that's a very long-term project... thanks, greg k-h
On Wed, May 19, 2021 at 08:04:59AM -0700, Jeff Johnson wrote: > On 2021-05-18 22:05, Greg Kroah-Hartman wrote: > > On Tue, May 18, 2021 at 03:00:44PM -0700, Jeff Johnson wrote: > > > On 2021-05-18 12:29, Jeff Johnson wrote: > > > Would still like guidance on if there is a recommended way to get a > > > dentry not associated with debugfs. > > > > What do you exactly mean by "not associated with debugfs"? > > > > And why are you passing a debugfs dentry to relay_open()? That feels > > really wrong and fragile. > > I don't know the history but the relay documentation tells us: > "If you want a directory structure to contain your relay files, > you should create it using the host filesystem’s directory > creation function, e.g. debugfs_create_dir()..." > > So my guess is that the original implementation followed that > advice. I see 5 clients of this functionality, and all 5 pass a > dentry returned from debugfs_create_dir(): > > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c, line 384 > drivers/net/wireless/ath/ath10k/spectral.c, line 534 > drivers/net/wireless/ath/ath11k/spectral.c, line 902 > drivers/net/wireless/ath/ath9k/common-spectral.c, line 1077 > kernel/trace/blktrace.c, line 549 Ah, that's just the "parent" dentry for the relayfs file. That's fine, not a big deal, debugfs will always provide a way for you to get that if needed. thanks, greg k-h
On Wed, May 19, 2021 at 08:57:00AM -0700, Jeff Johnson wrote: > On 2021-05-19 08:42, Greg Kroah-Hartman wrote: > > On Wed, May 19, 2021 at 08:04:59AM -0700, Jeff Johnson wrote: > > > On 2021-05-18 22:05, Greg Kroah-Hartman wrote: > > > > On Tue, May 18, 2021 at 03:00:44PM -0700, Jeff Johnson wrote: > > > > > On 2021-05-18 12:29, Jeff Johnson wrote: > > > > > Would still like guidance on if there is a recommended way to get a > > > > > dentry not associated with debugfs. > > > > > > > > What do you exactly mean by "not associated with debugfs"? > > > > > > > > And why are you passing a debugfs dentry to relay_open()? That feels > > > > really wrong and fragile. > > > > > > I don't know the history but the relay documentation tells us: > > > "If you want a directory structure to contain your relay files, > > > you should create it using the host filesystem’s directory > > > creation function, e.g. debugfs_create_dir()..." > > > > > > So my guess is that the original implementation followed that > > > advice. I see 5 clients of this functionality, and all 5 pass a > > > dentry returned from debugfs_create_dir(): > > > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c, line 384 > > > drivers/net/wireless/ath/ath10k/spectral.c, line 534 > > > drivers/net/wireless/ath/ath11k/spectral.c, line 902 > > > drivers/net/wireless/ath/ath9k/common-spectral.c, line 1077 > > > kernel/trace/blktrace.c, line 549 > > > > Ah, that's just the "parent" dentry for the relayfs file. That's fine, > > not a big deal, debugfs will always provide a way for you to get that if > > needed. > > Unless debugfs is disabled, like on Android, which is the real problem I'm > trying to solve. Then use some other filesystem to place your relay file in. A relay file is not a file that userspace should rely on for normal operation, so why do you need it at all? What tools/operation requires access to this file that systems without debugfs support is causing problems on? thanks, greg k-h
On 2021-05-19 09:03, Greg Kroah-Hartman wrote: > On Wed, May 19, 2021 at 08:57:00AM -0700, Jeff Johnson wrote: >> Unless debugfs is disabled, like on Android, which is the real problem >> I'm >> trying to solve. > > Then use some other filesystem to place your relay file in. A relay > file is not a file that userspace should rely on for normal operation, > so why do you need it at all? > > What tools/operation requires access to this file that systems without > debugfs support is causing problems on? (trimmed some lists) For the ath drivers (as well as the out-of-tree Android driver) it is being used for RF spectral scan which produces a large amount of data that is processed by a userspace analysis application. I've been looking for an alternate filesystem to use, but am not finding anything where it appears easy to get a parent dentry, so any clues for the clueless (me) would be appreciated. Jeff -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On Thu, May 20, 2021 at 03:16:35PM -0700, Jeff Johnson wrote: > On 2021-05-19 09:03, Greg Kroah-Hartman wrote: > > On Wed, May 19, 2021 at 08:57:00AM -0700, Jeff Johnson wrote: > > > Unless debugfs is disabled, like on Android, which is the real > > > problem I'm > > > trying to solve. > > > > Then use some other filesystem to place your relay file in. A relay > > file is not a file that userspace should rely on for normal operation, > > so why do you need it at all? > > > > What tools/operation requires access to this file that systems without > > debugfs support is causing problems on? > > (trimmed some lists) > > For the ath drivers (as well as the out-of-tree Android driver) it is being > used for RF spectral scan which produces a large amount of data that is > processed by a userspace analysis application. And that application requires root access in order to be able to read from the debugfs file today? That feels risky. And doesn't work for Android systems today, right? So what's wrong with just leaving it as-is, debugfs isn't going away on debug Android kernels, which is when you need to run this type of thing, only on the "user builds". And if it's really an issue, take that up with Google, it's their security policy that is driving this, not the community's requirements :) And note, moving files out of debugfs to elsewhere in the system circumvents the reasons that Google is not allowing debugfs anymore, which defeats the purpose entirely of that decision. Are you _sure_ that's a good idea for your users? > I've been looking for an alternate filesystem to use, but am not finding > anything where it appears easy to get a parent dentry, so any clues for the > clueless (me) would be appreciated. I'm not going to tell you how to abuse other filesystems in the kernel, but it shouldn't be all that hard if you really want to do it... good luck! greg k-h
On Tue, May 18, 2021 at 08:47:58PM +0300, Kalle Valo wrote: > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > > > There is no need to keep around the dentry pointers for the debugfs > > files as they will all be automatically removed when the subdir is > > removed. So save the space and logic involved in keeping them around by > > just getting rid of them entirely. > > > > By doing this change, we remove one of the last in-kernel user that was > > storing the result of debugfs_create_bool(), so that api can be cleaned > > up. > > > > Cc: Kalle Valo <kvalo@codeaurora.org> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > Cc: Chao Yu <chao@kernel.org> > > Cc: Leon Romanovsky <leon@kernel.org> > > Cc: linux-wireless@vger.kernel.org > > Cc: b43-dev@lists.infradead.org > > Cc: netdev@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > drivers/net/wireless/broadcom/b43/debugfs.c | 34 +++------------------ > > drivers/net/wireless/broadcom/b43/debugfs.h | 3 -- > > 2 files changed, 5 insertions(+), 32 deletions(-) > > > > Note, I can take this through my debugfs tree if wanted, that way I can > > clean up the debugfs_create_bool() api at the same time. Otherwise it's > > fine, I can wait until next -rc1 for that to happen. > > Yeah, it's best that you take this via your tree. > > Acked-by: Kalle Valo <kvalo@codeaurora.org> Thanks, will do! greg k-h
diff --git a/drivers/net/wireless/broadcom/b43/debugfs.c b/drivers/net/wireless/broadcom/b43/debugfs.c index 89a25aefb327..efa98444e3fb 100644 --- a/drivers/net/wireless/broadcom/b43/debugfs.c +++ b/drivers/net/wireless/broadcom/b43/debugfs.c @@ -643,24 +643,14 @@ bool b43_debug(struct b43_wldev *dev, enum b43_dyndbg feature) return enabled; } -static void b43_remove_dynamic_debug(struct b43_wldev *dev) -{ - struct b43_dfsentry *e = dev->dfsentry; - int i; - - for (i = 0; i < __B43_NR_DYNDBG; i++) - debugfs_remove(e->dyn_debug_dentries[i]); -} - static void b43_add_dynamic_debug(struct b43_wldev *dev) { struct b43_dfsentry *e = dev->dfsentry; #define add_dyn_dbg(name, id, initstate) do { \ e->dyn_debug[id] = (initstate); \ - e->dyn_debug_dentries[id] = \ - debugfs_create_bool(name, 0600, e->subdir, \ - &(e->dyn_debug[id])); \ + debugfs_create_bool(name, 0600, e->subdir, \ + &(e->dyn_debug[id])); \ } while (0) add_dyn_dbg("debug_xmitpower", B43_DBG_XMITPOWER, false); @@ -713,10 +703,9 @@ void b43_debugfs_add_device(struct b43_wldev *dev) #define ADD_FILE(name, mode) \ do { \ - e->file_##name.dentry = \ - debugfs_create_file(__stringify(name), \ - mode, e->subdir, dev, \ - &fops_##name.fops); \ + debugfs_create_file(__stringify(name), \ + mode, e->subdir, dev, \ + &fops_##name.fops); \ } while (0) @@ -746,19 +735,6 @@ void b43_debugfs_remove_device(struct b43_wldev *dev) e = dev->dfsentry; if (!e) return; - b43_remove_dynamic_debug(dev); - - debugfs_remove(e->file_shm16read.dentry); - debugfs_remove(e->file_shm16write.dentry); - debugfs_remove(e->file_shm32read.dentry); - debugfs_remove(e->file_shm32write.dentry); - debugfs_remove(e->file_mmio16read.dentry); - debugfs_remove(e->file_mmio16write.dentry); - debugfs_remove(e->file_mmio32read.dentry); - debugfs_remove(e->file_mmio32write.dentry); - debugfs_remove(e->file_txstat.dentry); - debugfs_remove(e->file_restart.dentry); - debugfs_remove(e->file_loctls.dentry); debugfs_remove(e->subdir); kfree(e->txstatlog.log); diff --git a/drivers/net/wireless/broadcom/b43/debugfs.h b/drivers/net/wireless/broadcom/b43/debugfs.h index 0bf437c86c67..6f6b500b8881 100644 --- a/drivers/net/wireless/broadcom/b43/debugfs.h +++ b/drivers/net/wireless/broadcom/b43/debugfs.h @@ -32,7 +32,6 @@ struct b43_txstatus_log { }; struct b43_dfs_file { - struct dentry *dentry; char *buffer; size_t data_len; }; @@ -70,8 +69,6 @@ struct b43_dfsentry { /* Enabled/Disabled list for the dynamic debugging features. */ bool dyn_debug[__B43_NR_DYNDBG]; - /* Dentries for the dynamic debugging entries. */ - struct dentry *dyn_debug_dentries[__B43_NR_DYNDBG]; }; bool b43_debug(struct b43_wldev *dev, enum b43_dyndbg feature);
There is no need to keep around the dentry pointers for the debugfs files as they will all be automatically removed when the subdir is removed. So save the space and logic involved in keeping them around by just getting rid of them entirely. By doing this change, we remove one of the last in-kernel user that was storing the result of debugfs_create_bool(), so that api can be cleaned up. Cc: Kalle Valo <kvalo@codeaurora.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Kees Cook <keescook@chromium.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: Chao Yu <chao@kernel.org> Cc: Leon Romanovsky <leon@kernel.org> Cc: linux-wireless@vger.kernel.org Cc: b43-dev@lists.infradead.org Cc: netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/net/wireless/broadcom/b43/debugfs.c | 34 +++------------------ drivers/net/wireless/broadcom/b43/debugfs.h | 3 -- 2 files changed, 5 insertions(+), 32 deletions(-) Note, I can take this through my debugfs tree if wanted, that way I can clean up the debugfs_create_bool() api at the same time. Otherwise it's fine, I can wait until next -rc1 for that to happen. v2: remove X/N from subject, it is a stand-alone patch.