linux-kernel - Re: [PATCH v3 2/3] Bluetooth: Add sysfs entry to enable/disable devcoredump

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>] [day] [month] [year] [list]

Date:   Fri, 8 Jul 2022 11:40:21 -0700
From:   Luiz Augusto von Dentz <luiz.dentz@...il.com>
To:     Manish Mandlik <mmandlik@...gle.com>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] Bluetooth: Add sysfs entry to enable/disable devcoredump

Hi Manish,

On Fri, Jul 8, 2022 at 11:30 AM Manish Mandlik <mmandlik@...gle.com> wrote:
>
> Hi Luiz,
>
> On Fri, Jul 8, 2022 at 10:24 AM Luiz Augusto von Dentz <luiz.dentz@...il.com> wrote:
>>
>> Hi Manish,
>>
>> On Thu, Jul 7, 2022 at 3:15 PM Manish Mandlik <mmandlik@...gle.com> wrote:
>> >
>> > Since device/firmware dump is a debugging feature, we may not need it
>> > all the time. Add a sysfs attribute to enable/disable bluetooth
>> > devcoredump capturing. The default state of this feature would be
>> > disabled and it can be enabled by echoing 1 to enable_coredump sysfs
>> > entry as follow:
>> >
>> > $ echo 1 > /sys/class/bluetooth/<device>/enable_coredump
>> >
>> > Signed-off-by: Manish Mandlik <mmandlik@...gle.com>
>> > ---
>> >
>> > Changes in v3:
>> > - New patch in the series to enable/disable feature via sysfs entry
>> >
>> >  net/bluetooth/hci_sysfs.c | 36 ++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 36 insertions(+)
>> >
>> > diff --git a/net/bluetooth/hci_sysfs.c b/net/bluetooth/hci_sysfs.c
>> > index 4e3e0451b08c..df0d54a5ae3f 100644
>> > --- a/net/bluetooth/hci_sysfs.c
>> > +++ b/net/bluetooth/hci_sysfs.c
>> > @@ -91,9 +91,45 @@ static void bt_host_release(struct device *dev)
>> >         module_put(THIS_MODULE);
>> >  }
>> >
>> > +#ifdef CONFIG_DEV_COREDUMP
>> > +static ssize_t enable_coredump_show(struct device *dev,
>> > +                                   struct device_attribute *attr,
>> > +                                   char *buf)
>> > +{
>> > +       struct hci_dev *hdev = to_hci_dev(dev);
>> > +
>> > +       return scnprintf(buf, 3, "%d\n", hdev->dump.enabled);
>> > +}
>> > +
>> > +static ssize_t enable_coredump_store(struct device *dev,
>> > +                                    struct device_attribute *attr,
>> > +                                    const char *buf, size_t count)
>> > +{
>> > +       struct hci_dev *hdev = to_hci_dev(dev);
>> > +
>> > +       /* Consider any non-zero value as true */
>> > +       if (simple_strtol(buf, NULL, 10))
>> > +               hdev->dump.enabled = true;
>> > +       else
>> > +               hdev->dump.enabled = false;
>> > +
>> > +       return count;
>> > +}
>> > +DEVICE_ATTR_RW(enable_coredump);
>> > +#endif
>> > +
>> > +static struct attribute *bt_host_attrs[] = {
>> > +#ifdef CONFIG_DEV_COREDUMP
>> > +       &dev_attr_enable_coredump.attr,
>> > +#endif
>> > +       NULL,
>> > +};
>> > +ATTRIBUTE_GROUPS(bt_host);
>> > +
>> >  static const struct device_type bt_host = {
>> >         .name    = "host",
>> >         .release = bt_host_release,
>> > +       .groups = bt_host_groups,
>> >  };
>> >
>> >  void hci_init_sysfs(struct hci_dev *hdev)
>> > --
>> > 2.37.0.rc0.161.g10f37bed90-goog
>>
>> It seems devcoredump.c creates its own sysfs entries so perhaps this
>> should be included there and documented in sysfs-devices-coredump.
>
> I deliberately created it here. We need to have this entry/switch per hci device, whereas the sysfs entry created by devcoredump.c disables the devcoredump feature entirely for anyone who's using it, so it can act as a global switch for the devcoredump.

We should probably check if there is a reason why this is not on per
device and anyway if seem wrong to each subsystem to come up with its
own sysfs entries when it could be easily generalized so the userspace
tools using those entries don't have to look into different entries
depending on the subsystem the device belongs.

>
>>
>> --
>> Luiz Augusto von Dentz
>
>
> Regards,
> Manish.



-- 
Luiz Augusto von Dentz