| 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
| ||
|
Message-ID: <nsaq3zungvyhuikz35arvxmle2fovxh422jpyqxuleh57ufqnk@bekeh7qr7y76>
Date: Mon, 2 Dec 2024 14:48:10 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Ekansh Gupta <quic_ekangupt@...cinc.com>
Cc: Greg KH <gregkh@...uxfoundation.org>, srinivas.kandagatla@...aro.org,
linux-arm-msm@...r.kernel.org, quic_bkumar@...cinc.com, linux-kernel@...r.kernel.org,
quic_chennak@...cinc.com, dri-devel@...ts.freedesktop.org, arnd@...db.de
Subject: Re: [PATCH v1 4/4] misc: fastrpc: Add debugfs support for fastrpc
On Mon, Dec 02, 2024 at 03:27:43PM +0530, Ekansh Gupta wrote:
>
>
> On 11/22/2024 12:23 AM, Dmitry Baryshkov wrote:
> > On Thu, Nov 21, 2024 at 12:12:17PM +0530, Ekansh Gupta wrote:
> >>
> >> On 11/18/2024 7:32 PM, Greg KH wrote:
> >>> On Mon, Nov 18, 2024 at 02:10:46PM +0530, Ekansh Gupta wrote:
> >>>> Add changes to support debugfs. The fastrpc directory will be
> >>>> created which will carry debugfs files for all fastrpc processes.
> >>>> The information of fastrpc user and channel contexts are getting
> >>>> captured as part of this change.
> >>>>
> >>>> Signed-off-by: Ekansh Gupta <quic_ekangupt@...cinc.com>
> >>>> ---
> >>>> drivers/misc/fastrpc/Makefile | 3 +-
> >>>> drivers/misc/fastrpc/fastrpc_debug.c | 156 +++++++++++++++++++++++++++
> >>>> drivers/misc/fastrpc/fastrpc_debug.h | 31 ++++++
> >>>> drivers/misc/fastrpc/fastrpc_main.c | 18 +++-
> >>>> 4 files changed, 205 insertions(+), 3 deletions(-)
> >>>> create mode 100644 drivers/misc/fastrpc/fastrpc_debug.c
> >>>> create mode 100644 drivers/misc/fastrpc/fastrpc_debug.h
> >>>>
> >>>> diff --git a/drivers/misc/fastrpc/Makefile b/drivers/misc/fastrpc/Makefile
> >>>> index 020d30789a80..4ff6b64166ae 100644
> >>>> --- a/drivers/misc/fastrpc/Makefile
> >>>> +++ b/drivers/misc/fastrpc/Makefile
> >>>> @@ -1,3 +1,4 @@
> >>>> # SPDX-License-Identifier: GPL-2.0
> >>>> obj-$(CONFIG_QCOM_FASTRPC) += fastrpc.o
> >>>> -fastrpc-objs := fastrpc_main.o
> >>>> \ No newline at end of file
> >>>> +fastrpc-objs := fastrpc_main.o \
> >>>> + fastrpc_debug.o
> >>> Only build this file if debugfs is enabled.
> >>>
> >>> And again, "debug.c"?
> >> I'll add change to build this only if debugfs is enabled. Going forward I have plans to add
> >> few more debug specific changes, maybe then I'll need to change the build rules again.
> >>>> diff --git a/drivers/misc/fastrpc/fastrpc_debug.c b/drivers/misc/fastrpc/fastrpc_debug.c
> >>>> new file mode 100644
> >>>> index 000000000000..cdb4fc6845a8
> >>>> --- /dev/null
> >>>> +++ b/drivers/misc/fastrpc/fastrpc_debug.c
> >>>> @@ -0,0 +1,156 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0
> >>>> +// Copyright (c) 2024 Qualcomm Innovation Center.
> >>>> +
> >>>> +#include <linux/debugfs.h>
> >>>> +#include <linux/seq_file.h>
> >>>> +#include "fastrpc_shared.h"
> >>>> +#include "fastrpc_debug.h"
> >>>> +
> >>>> +#ifdef CONFIG_DEBUG_FS
> >>> Please put the #ifdef in the .h file, not in the .c file.
> >> Ack
> >>>> +void fastrpc_create_user_debugfs(struct fastrpc_user *fl)
> >>>> +{
> >>>> + char cur_comm[TASK_COMM_LEN];
> >>>> + int domain_id, size;
> >>>> + char *debugfs_buf;
> >>>> + struct dentry *debugfs_dir = fl->cctx->debugfs_dir;
> >>>> +
> >>>> + memcpy(cur_comm, current->comm, TASK_COMM_LEN);
> >>>> + cur_comm[TASK_COMM_LEN-1] = '\0';
> >>>> + if (debugfs_dir != NULL) {
> >>>> + domain_id = fl->cctx->domain_id;
> >>>> + size = snprintf(NULL, 0, "%.10s_%d_%d_%d", cur_comm,
> >>>> + current->pid, fl->tgid, domain_id) + 1;
> >>>> + debugfs_buf = kzalloc(size, GFP_KERNEL);
> >>>> + if (debugfs_buf == NULL)
> >>>> + return;
> >>>> + /*
> >>>> + * Use HLOS process name, HLOS PID, fastrpc user TGID,
> >>>> + * domain_id in debugfs filename to create unique file name
> >>>> + */
> >>>> + snprintf(debugfs_buf, size, "%.10s_%d_%d_%d",
> >>>> + cur_comm, current->pid, fl->tgid, domain_id);
> >>>> + fl->debugfs_file = debugfs_create_file(debugfs_buf, 0644,
> >>>> + debugfs_dir, fl, &fastrpc_debugfs_fops);
> >>> Why are you saving the debugfs file? What do you need to do with it
> >>> that you can't just delete the whole directory, or look up the name
> >>> again in the future when removing it?
> >> fl structure is specific to a process using fastrpc driver. The reason to save
> >> this debugfs file is to delete is when the process releases fastrpc device.
> >> If the file is not deleted, it might flood multiple files in debugfs directory.
> >>
> >> As part of this change, only the file that is getting created by a process is
> >> getting removed when process is releasing device and I don't think we
> >> can clean up the whole directory at this point.
> > My 2c: it might be better to create a single file that conains
> > information for all the processes instead of that. Or use fdinfo data to
> > export process / FD information to userspace.
> Thanks for your review. The reason of not having single file for all processes is that
> I can run 100s of iteration for any process(say calculator) and every time the properties
> of the process can differ(like buffer, session etc.). For this reason, I'm creating and
> deleting the debugfs files for every process run.
>
> Do you see any advantage of using fdinfo over debugfs? I'm not sure if we can add all
> the information(like in debugfs) here.
Which information is actually useful / interesting for application
developers? If not for the fdinfo, I might still vote for a single file
rather than a pile of per-process data.
>
> --ekansh
> >
> >
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists