| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 24 Nov 2020 10:39:25 -0600
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Vinod Koul <vkoul@...nel.org>
Cc: Andy Gross <agross@...nel.org>, elder@...aro.org,
open list <linux-kernel@...r.kernel.org>,
"open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH] soc: qcom: Introduce debugfs interface to smem
On Tue 24 Nov 09:34 CST 2020, Vinod Koul wrote:
> On 22-11-20, 23:21, Bjorn Andersson wrote:
> > Every now and then it's convenient to be able to inspect the content of
> > SMEM items. Rather than carrying some hack locally let's upstream a
> > driver that when inserted exposes a debugfs interface for dumping
> > available items.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> > ---
> > drivers/soc/qcom/Kconfig | 7 +++
> > drivers/soc/qcom/Makefile | 1 +
> > drivers/soc/qcom/smem_debugfs.c | 102 ++++++++++++++++++++++++++++++++
> > 3 files changed, 110 insertions(+)
> > create mode 100644 drivers/soc/qcom/smem_debugfs.c
> >
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index 3dc3e3d61ea3..7e1dd6b3f33a 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -128,6 +128,13 @@ config QCOM_SMEM
> > The driver provides an interface to items in a heap shared among all
> > processors in a Qualcomm platform.
> >
> > +config QCOM_SMEM_DEBUGFS
> > + tristate "Qualcomm Shared Memory Manager (SMEM) DebugFS interface"
> > + depends on QCOM_SMEM
> > + depends on DEBUG_FS
> > + help
> > + Provides a debugfs interface for inspecting SMEM.
>
> Do we need additional debugfs entry, maybe better to depend on DEBUG_FS
> being enabled and this file part of QCOM_SMEM?
>
We don't need this in any form of production system, so rather than
tainting qcom_smem.c I put it in a separate driver that isn't even
automatically loaded.
> > +
> > config QCOM_SMD_RPM
> > tristate "Qualcomm Resource Power Manager (RPM) over SMD"
> > depends on ARCH_QCOM || COMPILE_TEST
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index 93392d9dc7f7..632eefc5a897 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -15,6 +15,7 @@ qcom_rpmh-y += rpmh-rsc.o
> > qcom_rpmh-y += rpmh.o
> > obj-$(CONFIG_QCOM_SMD_RPM) += smd-rpm.o
> > obj-$(CONFIG_QCOM_SMEM) += smem.o
> > +obj-$(CONFIG_QCOM_SMEM_DEBUGFS) += smem_debugfs.o
> > obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> > obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> > obj-$(CONFIG_QCOM_SMSM) += smsm.o
> > diff --git a/drivers/soc/qcom/smem_debugfs.c b/drivers/soc/qcom/smem_debugfs.c
> > new file mode 100644
> > index 000000000000..11ef29a0cada
> > --- /dev/null
> > +++ b/drivers/soc/qcom/smem_debugfs.c
> > @@ -0,0 +1,102 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020, Linaro Ltd.
> > + */
> > +
> > +#include <linux/debugfs.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/soc/qcom/smem.h>
> > +
> > +struct smem_debugfs {
> > + struct dentry *root;
> > +};
> > +
> > +static int smem_debugfs_item_show(struct seq_file *seq, void *p)
> > +{
> > + unsigned long data = (unsigned long)seq->private;
> > + unsigned long item = data & 0xffff;
> > + unsigned long host = data >> 16;
> > + size_t len;
> > + void *ptr;
> > +
> > + ptr = qcom_smem_get(host, item, &len);
> > + if (IS_ERR(ptr))
> > + return PTR_ERR(ptr);
> > +
> > + seq_hex_dump(seq, "", DUMP_PREFIX_OFFSET, 16, 1, ptr, len, true);
> > +
> > + return 0;
> > +}
> > +
> > +static int smem_debugfs_item_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, smem_debugfs_item_show, inode->i_private);
> > +}
> > +
> > +static const struct file_operations smem_debugfs_item_ops = {
> > + .open = smem_debugfs_item_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
>
> How about using DEFINE_SHOW_ATTRIBUTE() instead? That will help cut down
> this boiler plate code..
>
Forgot about that, thank you.
> > +
> > +static int smem_debugfs_rescan(struct seq_file *seq, void *p)
> > +{
> > + struct dentry *root = seq->private;
> > + unsigned long item;
> > + unsigned long host;
> > + unsigned long data;
> > + char name[10];
> > + char *ptr;
> > +
> > + for (host = 0; host < 10; host++) {
> > + for (item = 0; item < 512; item++) {
> > + ptr = qcom_smem_get(host, item, NULL);
> > + if (IS_ERR(ptr))
> > + continue;
> > +
> > + sprintf(name, "%ld-%ld", host, item);
> > +
> > + data = host << 16 | item;
> > + debugfs_create_file(name, 0400, root,
> > + (void *)data, &smem_debugfs_item_ops);
>
> So IIUC user invokes scan file which creates additional files, right?
> Additional invoke will do that as well..?
>
Yes, so if you run it a second time debugfs_create_file() will fail for
any items that was present during the last invocation.
I did consider adding some logic to keep track of what items we have
already registered, but it is just debugging code and given that after a
few second of operations the set of items has stabilized you typically
don't run this repeatedly.
So I don't think it's worth the memory occupied by an idr or 5000+ bits
in a map.
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int smem_debugfs_rescan_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, smem_debugfs_rescan, inode->i_private);
> > +}
> > +
> > +static const struct file_operations smem_debugfs_rescan_ops = {
> > + .open = smem_debugfs_rescan_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
>
> Here as well?
>
Will fix.
Thank you,
Bjorn
> > +
> > +static struct dentry *smem_debugfs_root;
> > +
> > +static int __init qcom_smem_debugfs_init(void)
> > +{
> > + smem_debugfs_root = debugfs_create_dir("qcom_smem", NULL);
> > + debugfs_create_file("rescan", 0400, smem_debugfs_root,
> > + smem_debugfs_root, &smem_debugfs_rescan_ops);
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit qcom_smem_debugfs_exit(void)
> > +{
> > + debugfs_remove_recursive(smem_debugfs_root);
> > +}
> > +
> > +module_init(qcom_smem_debugfs_init);
> > +module_exit(qcom_smem_debugfs_exit);
> > +
> > +MODULE_DESCRIPTION("Qualcomm SMEM debugfs driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.29.2
>
> --
> ~Vinod
Powered by blists - more mailing lists