On Thu, Oct 17, 2024 at 5:12 AM Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> wrote: > > Hi Anish and Mukesh, > > On 10/16/24 18:37, anish kumar wrote: > > On Tue, Oct 15, 2024 at 10:45 PM Mukesh Ojha <quic_mojha@xxxxxxxxxxx> wrote: > >> > >> On Tue, Oct 15, 2024 at 03:28:08PM -0700, anish kumar wrote: > >>> On Tue, Oct 15, 2024 at 12:28 PM Mukesh Ojha <quic_mojha@xxxxxxxxxxx> wrote: > >>>> > >>>> On Tue, Oct 15, 2024 at 10:59:12AM -0700, anish kumar wrote: > >>>>> On Tue, Oct 15, 2024 at 9:26 AM Mukesh Ojha <quic_mojha@xxxxxxxxxxx> wrote: > >>>>>> > >>>>>> There is NULL pointer issue observed if from Process A where hid device > >>>>>> being added which results in adding a led_cdev addition and later a > >>>>>> another call to access of led_cdev attribute from Process B can result > >>>>>> in NULL pointer issue. > >>>>> > >>>>> Which pointer is NULL? Call stack shows that dualshock4_led_get_brightness > >>>>> function could be culprit? > >>>> > >>>> in dualshock4_led_get_brightness()[1], led->dev is NULL here, as [2] > >>>> is not yet completed. > >>>> > >>>> [1] > >>>> struct hid_device *hdev = to_hid_device(led->dev->parent); > >>>> > >>>> [2] > >>>> led_cdev->dev = device_create_with_groups(&leds_class, parent, 0, > >>>> led_cdev, led_cdev->groups, "%s", final_name); > >>>> > >>>>> > >>>>>> > >>>>>> Use mutex led_cdev->led_access to protect access to led->cdev and its > >>>>>> attribute inside brightness_show(). > >>>>> > >>>>> I don't think it is needed here because it is just calling the led driver > >>>>> callback and updating the brightness. So, why would we need to serialize > >>>>> that using mutex? Maybe the callback needs some debugging. > >>>>> I'm curious if it is ready by the time the callback is invoked. > >>>> > >>>> Because, we should not be allowed to access led_cdev->dev as it is not > >>>> completed and since, brightness_store() has this lock brightness_show() > >>>> should also have this as we are seeing the issue without it. > >>>> > >>>> I hope, above might have answered your question. > >>>> > >>>> -Mukesh > >>>>> > >>>>>> > >>>>>> Process A Process B > >>>>>> > >>>>>> kthread+0x114 > >>>>>> worker_thread+0x244 > >>>>>> process_scheduled_works+0x248 > >>>>>> uhid_device_add_worker+0x24 > >>>>>> hid_add_device+0x120 > >>>>>> device_add+0x268 > >>>>>> bus_probe_device+0x94 > >>>>>> device_initial_probe+0x14 > >>>>>> __device_attach+0xfc > >>>>>> bus_for_each_drv+0x10c > >>>>>> __device_attach_driver+0x14c > >>>>>> driver_probe_device+0x3c > >>>>>> __driver_probe_device+0xa0 > >>>>>> really_probe+0x190 > >>>>>> hid_device_probe+0x130 > >>>>>> ps_probe+0x990 > >>>>>> ps_led_register+0x94 > >>>>>> devm_led_classdev_register_ext+0x58 > >>>>>> led_classdev_register_ext+0x1f8 > >>>>>> device_create_with_groups+0x48 > >>>>>> device_create_groups_vargs+0xc8 > >>>>>> device_add+0x244 > >>>>>> kobject_uevent+0x14 > >>>>>> kobject_uevent_env[jt]+0x224 > >>>>>> mutex_unlock[jt]+0xc4 > >>>>>> __mutex_unlock_slowpath+0xd4 > >>>>>> wake_up_q+0x70 > >>>>>> try_to_wake_up[jt]+0x48c > >>>>>> preempt_schedule_common+0x28 > >>>>>> __schedule+0x628 > >>>>>> __switch_to+0x174 > >>>>>> el0t_64_sync+0x1a8/0x1ac > >>>>>> el0t_64_sync_handler+0x68/0xbc > >>>>>> el0_svc+0x38/0x68 > >>>>>> do_el0_svc+0x1c/0x28 > >>>>>> el0_svc_common+0x80/0xe0 > >>>>>> invoke_syscall+0x58/0x114 > >>>>>> __arm64_sys_read+0x1c/0x2c > >>>>>> ksys_read+0x78/0xe8 > >>>>>> vfs_read+0x1e0/0x2c8 > >>>>>> kernfs_fop_read_iter+0x68/0x1b4 > >>>>>> seq_read_iter+0x158/0x4ec > >>>>>> kernfs_seq_show+0x44/0x54 > >>>>>> sysfs_kf_seq_show+0xb4/0x130 > >>>>>> dev_attr_show+0x38/0x74 > >>>>>> brightness_show+0x20/0x4c > >>>>>> dualshock4_led_get_brightness+0xc/0x74 > >>>>>> > >>>>>> [ 3313.874295][ T4013] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000060 > >>>>>> [ 3313.874301][ T4013] Mem abort info: > >>>>>> [ 3313.874303][ T4013] ESR = 0x0000000096000006 > >>>>>> [ 3313.874305][ T4013] EC = 0x25: DABT (current EL), IL = 32 bits > >>>>>> [ 3313.874307][ T4013] SET = 0, FnV = 0 > >>>>>> [ 3313.874309][ T4013] EA = 0, S1PTW = 0 > >>>>>> [ 3313.874311][ T4013] FSC = 0x06: level 2 translation fault > >>>>>> [ 3313.874313][ T4013] Data abort info: > >>>>>> [ 3313.874314][ T4013] ISV = 0, ISS = 0x00000006, ISS2 = 0x00000000 > >>>>>> [ 3313.874316][ T4013] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 > >>>>>> [ 3313.874318][ T4013] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > >>>>>> [ 3313.874320][ T4013] user pgtable: 4k pages, 39-bit VAs, pgdp=00000008f2b0a000 > >>>>>> .. > >>>>>> > >>>>>> [ 3313.874332][ T4013] Dumping ftrace buffer: > >>>>>> [ 3313.874334][ T4013] (ftrace buffer empty) > >>>>>> .. > >>>>>> .. > >>>>>> [ dd3313.874639][ T4013] CPU: 6 PID: 4013 Comm: InputReader > >>>>>> [ 3313.874648][ T4013] pc : dualshock4_led_get_brightness+0xc/0x74 > >>>>>> [ 3313.874653][ T4013] lr : led_update_brightness+0x38/0x60 > >>>>>> [ 3313.874656][ T4013] sp : ffffffc0b910bbd0 > >>>>>> .. > >>>>>> .. > >>>>>> [ 3313.874685][ T4013] Call trace: > >>>>>> [ 3313.874687][ T4013] dualshock4_led_get_brightness+0xc/0x74 > >>>>>> [ 3313.874690][ T4013] brightness_show+0x20/0x4c > >>>>>> [ 3313.874692][ T4013] dev_attr_show+0x38/0x74 > >>>>>> [ 3313.874696][ T4013] sysfs_kf_seq_show+0xb4/0x130 > >>>>>> [ 3313.874700][ T4013] kernfs_seq_show+0x44/0x54 > >>>>>> [ 3313.874703][ T4013] seq_read_iter+0x158/0x4ec > >>>>>> [ 3313.874705][ T4013] kernfs_fop_read_iter+0x68/0x1b4 > >>>>>> [ 3313.874708][ T4013] vfs_read+0x1e0/0x2c8 > >>>>>> [ 3313.874711][ T4013] ksys_read+0x78/0xe8 > >>>>>> [ 3313.874714][ T4013] __arm64_sys_read+0x1c/0x2c > >>>>>> [ 3313.874718][ T4013] invoke_syscall+0x58/0x114 > >>>>>> [ 3313.874721][ T4013] el0_svc_common+0x80/0xe0 > >>>>>> [ 3313.874724][ T4013] do_el0_svc+0x1c/0x28 > >>>>>> [ 3313.874727][ T4013] el0_svc+0x38/0x68 > >>>>>> [ 3313.874730][ T4013] el0t_64_sync_handler+0x68/0xbc > >>>>>> [ 3313.874732][ T4013] el0t_64_sync+0x1a8/0x1ac > >>>>>> > >>>>>> Signed-off-by: Mukesh Ojha <quic_mojha@xxxxxxxxxxx> > >>>>>> --- > >>>>>> drivers/leds/led-class.c | 3 ++- > >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > >>>>>> index 06b97fd49ad9..e3cb93f19c06 100644 > >>>>>> --- a/drivers/leds/led-class.c > >>>>>> +++ b/drivers/leds/led-class.c > >>>>>> @@ -30,8 +30,9 @@ static ssize_t brightness_show(struct device *dev, > >>>>>> { > >>>>>> struct led_classdev *led_cdev = dev_get_drvdata(dev); > >>>>>> > >>>>>> - /* no lock needed for this */ > >>> > >>> just get rid of the above comment then. > >> > >> If you notice, it is already removed (-) . > >> > >>> > >>> Also, the comment below in file leds.h > >>> needs an update as originally the idea for this mutex lock was to > >>> provide quick feedback to userspace based on this commit > >>> https://github.com/torvalds/linux/commit/acd899e4f3066b6662f6047da5b795cc762093cb > >>> > >>> Basically a comment somewhere so that when a new attribute > >>> gets added, it doesn't make the same mistake of not using the mutex > >>> and run into the same issue. > >>> > >>> /* Ensures consistent access to the LED Flash Class device */ > >>> struct mutex led_access; > >> > >> Thanks for accepting that it is an issue. > >> I think, comment is very obvious actually the patch you mentioned should > >> be in fixes tag as it introduced the lock but did not protect the show > >> while it does it for store. > > > > Yes, but that patch was added for supporting flash class > > device and wasn't explicitly to take care of the scenario that you > > are trying to handle and the above comment in leds.h states the same. > > Correct. led_access mutex was introduced to add support for preventing > any LED class device state changes originating from sysfs while > v4l2_flash wrapper owns the device. > > Since the inception of LED subsystem all the locking was deemed to be > the responsibility of every single LED class driver and initially sysfs > attr callbacks didn't have any locking. After some time when LED core > started to grow it turned out that it was required to lock the LED class > initialization sequence, so as not to give the userspace an opportunity > to set LED brightness on not fully initialized device, which was > introduced in [0]. led_access mutex was already in place so it was used. > However as you noticed, it is not used consistently across all LED class > sysfs attrs callbacks. > > Since brightness_show() does not acquire led_access mutex it is still > possible to call brightness_get op when LED class initialization > sequence is not yet finished. > > Still, I'd propose to first narrow down the issue and figure out what > actually causes NULL pointer dereference, as it apparently > originates from dualshock4_led_get_brightness and not from LED core. Mukesh already explained the issue in earlier emails but here is the gist anyway. led_cdev->dev = device_create_with_groups(&leds_class, parent, 0, led_cdev, led_cdev->groups, "%s", final_name); If you look at the above code, device_create_with_groups function can create all the sysfs and before it returns and assigns led_cdev->dev pointer, those sysfs callback can get triggered and if the callback accesses led_cdev->dev this variable, it will crash as it is not yet assigned. In my opinion, we just have to put a proper comment in attribute creation part so that if a new attribute gets added it uses the lock. > > I bet that the driver is not fully initialized up to the point when > devm_led_classdev_register_ext() is called in it. > > > > > I think we should modify that comment and state clearly that > > the aforementioned mutex is also to handle access to led_cdev->dev. > > Either here in this .h or where attributes are defined, so that new attributes > > that get added doesn't suffer from the same bug. > > > > led_trigger_set also this function also suffers from the same bug so you > > need to handle it the same way. > > led_trigger_set() is already called with led_access mutex held in > led_trigger_write(), i.e. from "trigger" sysfs attr. makes sense. > > >> > >> Fixes: acd899e4f306 ("leds: implement sysfs interface locking mechanism") > >> > >> -Mukesh > >>> > >>> > >>>>> > >>>>>>> also you missed this. > >>>>> > >>>>>> + mutex_lock(&led_cdev->led_access); > >>>>>> led_update_brightness(led_cdev); > >>>>>> + mutex_unlock(&led_cdev->led_access); > >>>>>> > >>>>>> return sprintf(buf, "%u\n", led_cdev->brightness); > >>>>>> } > >>>>>> -- > >>>>>> 2.34.1 > >>>>>> > >>>>>> > > > > [0] > https://lore.kernel.org/linux-leds/20180523222221.27621-1-lhenriques@xxxxxxxx/ > > -- > Best regards, > Jacek Anaszewski