netdev - Re: [PATCH v1 1/2] Bluetooth: coredump: Add hci_devcd

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <AS4PR04MB969274E6E1CD42BC80C1BE61E742A@AS4PR04MB9692.eurprd04.prod.outlook.com>
Date: Fri, 4 Jul 2025 07:23:39 +0000
From: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
CC: "marcel@...tmann.org" <marcel@...tmann.org>, "johan.hedberg@...il.com"
	<johan.hedberg@...il.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>, "kuba@...nel.org"
	<kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"horms@...nel.org" <horms@...nel.org>, "linux-bluetooth@...r.kernel.org"
	<linux-bluetooth@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, Amitkumar Karwar <amitkumar.karwar@....com>, Sherry
 Sun <sherry.sun@....com>, Manjeet Gupta <manjeet.gupta@....com>
Subject: Re: [PATCH v1 1/2] Bluetooth: coredump: Add hci_devcd_unregister()
 for cleanup

Hi Luiz,

Thank you for reviewing this patch.

> On Thu, Jul 3, 2025 at 9:17 AM Neeraj Sanjay Kale
> <neeraj.sanjaykale@....com> wrote:
> >
> > This adds hci_devcd_unregister() which can be called when driver is
> > removed, which will cleanup the devcoredump data and cancel delayed
> > dump_timeout work.
> >
> > With BTNXPUART driver, it is observed that after FW dump, if driver is
> > removed and re-loaded, it creates hci1 interface instead of hci0
> > interface.
> >
> > But after DEVCD_TIMEOUT (5 minutes) if driver is re-loaded, hci0 is
> > created. This is because after FW dump, hci0 is not unregistered
> > properly for DEVCD_TIMEOUT.
> >
> > With this patch, BTNXPUART is able to create hci0 after every FW dump
> > and driver reload.
> >
> > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
> > ---
> >  include/net/bluetooth/coredump.h | 3 +++
> >  net/bluetooth/coredump.c         | 8 ++++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/include/net/bluetooth/coredump.h
> > b/include/net/bluetooth/coredump.h
> > index 72f51b587a04..bc8856e4bfe7 100644
> > --- a/include/net/bluetooth/coredump.h
> > +++ b/include/net/bluetooth/coredump.h
> > @@ -66,6 +66,7 @@ void hci_devcd_timeout(struct work_struct *work);
> >
> >  int hci_devcd_register(struct hci_dev *hdev, coredump_t coredump,
> >                        dmp_hdr_t dmp_hdr, notify_change_t
> > notify_change);
> > +void hci_devcd_unregister(struct hci_dev *hdev);
> >  int hci_devcd_init(struct hci_dev *hdev, u32 dump_size);  int
> > hci_devcd_append(struct hci_dev *hdev, struct sk_buff *skb);  int
> > hci_devcd_append_pattern(struct hci_dev *hdev, u8 pattern, u32 len);
> > @@ -85,6 +86,8 @@ static inline int hci_devcd_register(struct hci_dev
> *hdev, coredump_t coredump,
> >         return -EOPNOTSUPP;
> >  }
> >
> > +static inline void hci_devcd_unregister(struct hci_dev *hdev) {}
> > +
> >  static inline int hci_devcd_init(struct hci_dev *hdev, u32 dump_size)
> > {
> >         return -EOPNOTSUPP;
> > diff --git a/net/bluetooth/coredump.c b/net/bluetooth/coredump.c index
> > 819eacb38762..dd7bd40e3eba 100644
> > --- a/net/bluetooth/coredump.c
> > +++ b/net/bluetooth/coredump.c
> > @@ -442,6 +442,14 @@ int hci_devcd_register(struct hci_dev *hdev,
> > coredump_t coredump,  }  EXPORT_SYMBOL(hci_devcd_register);
> >
> > +void hci_devcd_unregister(struct hci_dev *hdev) {
> > +       cancel_delayed_work(&hdev->dump.dump_timeout);
> > +       skb_queue_purge(&hdev->dump.dump_q);
> > +       dev_coredump_put(&hdev->dev);
> > +}
> > +EXPORT_SYMBOL_GPL(hci_devcd_unregister);
> 
> The fact that the dump lives inside hdev is sort of the source of these
> problems, specially if the dumps are not HCI traffic it might be better off
> having the driver control its lifetime and not use
> hdev->workqueue to schedule it.
Are you are talking about "hdev->dump.dump_timeout"? it does not control the dump lifetime.
It simply makes sure that once FW dump is started, it should be complete within "dump_timeout" seconds.

The actual cleaning up of dump data is done by the " devcd->del_wk" which is delay-scheduled by 5 minutes in dev_coredumpm_timeout(), which is part of the devcoredump base.

Sure, with some modification, the driver can control the dump lifetime instead of hardcode DEVCD_TIMEOUT, but during driver exit, there is a need for "dev_coredump_put()" API to be called anyway.

Please let me know your thoughts on this.

> 
> >  static inline bool hci_devcd_enabled(struct hci_dev *hdev)  {
> >         return hdev->dump.supported;
> > --

Thanks,
Neeraj