linux-kernel - 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:
 <AS4PR04MB96920BF303B02AF7542A09A3E723A@AS4PR04MB9692.eurprd04.prod.outlook.com>
Date: Mon, 4 Aug 2025 09:50:32 +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,

A follow-up on my previous reply.

Looks like the dump lifetime is fixed/hardcoded to 5 minutes by the devcoredump base framework on purpose. Any change to this framework, to allow btnxpuart driver control the dump lifetime may not get approved by devcoredump maintainers.
https://github.com/bluez/bluetooth-next/commit/3b9c181bcde8555ca81b2394c2dc2201cefc2dd4#diff-dd2971a4abc5dbad2b03c42f25b2d8ef9d9417189070012500f315ee22fd17f9

Anyways, for Bluetooth drivers that relay on hci_devcd layer, calling the new hci_devcd_unregister() API fixes the issue mentioned in the commit message, properly cleaning up the dump data if driver is removed before the dump lifetime is complete, no matter who controls the dump lifetime.

Please let me know if I'm missing something.

Thanks,
Neeraj
 
> 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