linux-kernel - Re: [PATCH] HID: wacom: Make use of the helper function devm_add_action_or

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

Message-ID: <CAF8JNhLF8_f1x1K52ay_cmkKqpNiY7P4kMwt=ia6ws9Yd9uoNQ@mail.gmail.com>
Date:   Sun, 17 Oct 2021 14:58:47 -0700
From:   Ping Cheng <pinglinux@...il.com>
To:     Jiri Kosina <jkosina@...e.cz>,
        Benjamin Tissoires <benjamin.tissoires@...il.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Jason Gerecke <killertofu@...il.com>,
        Jiri Kosina <jikos@...nel.org>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Linux Input <linux-input@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Aaron Skomra <skomra@...il.com>,
        "Dickens, Joshua" <joshua.dickens@...om.com>, caihuoqing@...du.com
Subject: Re: [PATCH] HID: wacom: Make use of the helper function devm_add_action_or_reset()

I tested the set of two patches. I didn't see any issues with them
applied. But, while reviewing the patches, I noticed a minor logic
mismatch between the current patch and the original code. I'd hope at
least one of the maintainers (Jiri, Benjamin, or Dimitry) reviews this
patch, especially the part that I commented below, to make sure that
we don't trigger any race condition.

Thank you Huoqing, Jason, and the maintainer team!

> > From 7adc05783c7e3120028d0d089bea224903c24ccd Mon Sep 17 00:00:00 2001
> > From: Jason Gerecke <jason.gerecke@...om.com>
> > Date: Thu, 14 Oct 2021 07:31:31 -0700
> > Subject: [PATCH] RFC: HID: wacom: Shrink critical section in
> >  `wacom_add_shared_data`
> >
> > The size of the critical section in this function appears to be larger
> > than necessary. The `wacom_udev_list_lock` exists to ensure that one
> > interface cannot begin checking if a shared object exists while a second
> > interface is doing the same (otherwise both could determine that that no
> > object exists yet and create their own independent objects rather than
> > sharing just one). It should be safe for the critical section to end
> > once a fresly-allocated shared object would be found by other threads
> > (i.e., once it has been added to `wacom_udev_list`, which is looped
> > over by `wacom_get_hdev_data`).
> >
> > This commit is a necessary pre-requisite for a later change to swap the
> > use of `devm_add_action` with `devm_add_action_or_reset`, which would
> > otherwise deadlock in its error case.
> >
> > Signed-off-by: Jason Gerecke <jason.gerecke@...om.com>
> > ---
> >  drivers/hid/wacom_sys.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index 93f49b766376..62f50e4b837d 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -881,8 +881,8 @@ static int wacom_add_shared_data(struct hid_device *hdev)
> >       if (!data) {
> >               data = kzalloc(sizeof(struct wacom_hdev_data), GFP_KERNEL);
> >               if (!data) {
> > -                     retval = -ENOMEM;
> > -                     goto out;
> > +                     mutex_unlock(&wacom_udev_list_lock);
> > +                     return -ENOMEM;
> >               }
> >
> >               kref_init(&data->kref);
> > @@ -890,11 +890,12 @@ static int wacom_add_shared_data(struct hid_device *hdev)
> >               list_add_tail(&data->list, &wacom_udev_list);
> >       }
> >
> > +     mutex_unlock(&wacom_udev_list_lock);
> > +
> >       wacom_wac->shared = &data->shared;
> >
> >       retval = devm_add_action(&hdev->dev, wacom_remove_shared_data, wacom);
> >       if (retval) {
> > -             mutex_unlock(&wacom_udev_list_lock);

The mutex_unlock was called after devm_add_action is finished, whether
it is a failure or success. The new code calls mutex_unlock before
devm_add_action is executed. Is that ok? If there is no concern from
the maintainers, the patch has been:

Reviewed-by: Ping Cheng <ping.cheng@...om.com>
Tested-by: Ping Cheng <ping.cheng@...om.com>

Cheers,
Ping

> >               wacom_remove_shared_data(wacom);
> >               return retval;
> >       }
> > @@ -904,8 +905,6 @@ static int wacom_add_shared_data(struct hid_device *hdev)
> >       else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
> >               wacom_wac->shared->pen = hdev;
> >
> > -out:
> > -     mutex_unlock(&wacom_udev_list_lock);
> >       return retval;
> >  }
> >
> > --
> > 2.33.0
> >
>