linux-kernel - Re: [BUG 4.9] New led trigger usbport gets the kernel to panic

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]

Date:   Tue, 6 Dec 2016 18:26:20 +0100
From:   Pavel Machek <pavel@....cz>
To:     Ralph Sennhauser <ralph.sennhauser@...il.com>,
        linux-leds@...r.kernel.org, j.anaszewski@...sung.com,
        rpurdie@...ys.net
Cc:     Rafał Miłecki <rafal@...ecki.pl>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Rafał Miłecki <zajec5@...il.com>
Subject: Re: [BUG 4.9] New led trigger usbport gets the kernel to panic

On Fri 2016-12-02 09:48:18, Ralph Sennhauser wrote:
> On Thu, 1 Dec 2016 17:56:07 +0100
> Rafał Miłecki <rafal@...ecki.pl> wrote:
> 
> > On 12/01/2016 03:28 PM, Ralph Sennhauser wrote:
> > > Below the oops with your debug patch applied.
> > >
> > > (...)
> > >
> > > root@...1900acs:/# cd sys/class/leds/pca963x\:shelby\:white\:usb2/
> > > root@...1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2#
> > > echo usbport > trigger [  124.727665] leds
> > > pca963x:shelby:white:usb2: [usbport_trig_add_port] port:dd708240
> > > [  124.735266] leds pca963x:shelby:white:usb2:
> > > [usbport_trig_add_port] port->port_name:usb1-port1
> > > port->data:dd708140 [  124.745671] leds pca963x:shelby:white:usb2:
> > > [usbport_trig_add_port] port:dd7083c0 [  124.753194] leds
> > > pca963x:shelby:white:usb2: [usbport_trig_add_port]
> > > port->port_name:usb2-port1 port->data:dd708140 [  124.763594] leds
> > > pca963x:shelby:white:usb2: [usbport_trig_add_port] port:dd708300
> > > [  124.771114] leds pca963x:shelby:white:usb2:
> > > [usbport_trig_add_port] port->port_name:usb3-port1
> > > port->data:dd708140
> > > root@...1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2#
> > > echo 1 > ports/usb2-port1[  171.649751] leds
> > > pca963x:shelby:white:usb2: [usbport_trig_port_store] buf:1
> > > [  171.649751]  size:2
> > >
> > > [  171.660160] leds pca963x:shelby:white:usb2:
> > > [usbport_trig_port_store] port:dd7083c0 [  171.668103] leds
> > > pca963x:shelby:white:usb2: [usbport_trig_port_store]
> > > port->port_name:usb2-port1 port->data:dd708140 [  171.678678]
> > > [usbport_trig_update_count] usbport_data->count:0 [  171.684457]
> > > [usbport_trig_update_count] usbport_data->count:0 [  171.690253]
> > > Unable to handle kernel NULL pointer dereference at virtual address
> > > 00000000  
> > 
> > Oh, so this happens a bit later than I expected or I could read from
> > the backtrace. Anyway this debugging was still helpful, I think I can
> > see a possible problem.
> > 
> > So most likely the crash happens at the:
> > led_cdev->brightness_set(led_cdev, ...);
> > call. I'm not sure what I was thinking when writing that code. It
> > looks wrong.
> > 
> > The thing is some LEDs (drivers) don't provide brightness_set op.
> > It's fine, we should just use brightness_set_blocking op when that
> > happens. Of course kernel has proper helpers for that, we don't have
> > to worry about the choice of op or scheduling the work. I have no
> > idea why I didn't use a proper helper in the first place.
> > 
> > So we should simply replace above call with one of following ones:
> > 1) led_set_brightness(led_cdev, ...);
> > 2) led_set_brightness_nosleep(led_cdev, ...);
> > 3) led_set_brightness_sync(led_cdev, ...);
> > 
> > I still have to check which one is correct. In theory we don't deal
> > blinking at this point so we shouldn't need to use led_set_brightness.
> > 
> > led_set_brightness_nosleep looks like the most likely correct choice.
> > 
> > led_set_brightness_sync requires brightness_set_blocking which is not
> > always present so most likely we don't want this one.
> > 
> > 
> > If you have some free time and you want to play with this, please
> > replace led_cdev->brightness_set
> > with
> > led_set_brightness_nosleep
> > and give it a try. This should fix crashes for you.
> > 
> > I'll look at this again during next days.
> 
> Hi Rafał
> 
> I just gave 2) led_set_brightness_nosleep a try. The kernel no longer
> crashes and the led does work as expected.

Do you have any patch that needs to be applied?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)