linux-kernel - Re: + softirq-fix-tasklet_kill-and-its-users.patch added to -mm tree

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:   Wed, 21 Sep 2016 19:31:30 -0700
From:   Santosh Shilimkar <santosh.shilimkar@...cle.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        akpm@...ux-foundation.org
Cc:     ssantosh@...nel.org, davem@...emloft.net,
        giovanni.cabiddu@...el.com, gregkh@...uxfoundation.org,
        herbert@...dor.apana.org.au, isdn@...ux-pingi.de, mingo@...e.hu,
        pebolle@...cali.nl, peterz@...radead.org,
        salvatore.benedetto@...el.com, tadeusz.struk@...el.com,
        tglx@...utronix.de, mm-commits@...r.kernel.org,
        linux-kernel@...r.kernel.org, sfr@...b.auug.org.au,
        linux-next@...r.kernel.org, sergey.senozhatsky@...il.com
Subject: Re: + softirq-fix-tasklet_kill-and-its-users.patch added to -mm tree

On 9/21/2016 5:42 PM, Sergey Senozhatsky wrote:
> Hello,
>
> On (09/21/16 10:23), Santosh Shilimkar wrote:

[...]

>> Am assuming one of the driver in your test is using the DECLARE_TASKLET
>> to init the tasklet and killed by tasklet_kill() which leaves that
>> tasklet to be still scheduled by tasklet action.
>
> yes, vt does something like this (kbd_bh).
>
>> Can you please try below patch and see if you still see the issue ?
>> Attaching the same, just in case mailer eat the tabs.
>
> hm, didn't completely fix it. the vt is now happy, unlike usbnet.
Good that vt works now.

> and the usbnet case is rather alarming.
>
>
> looking at usbnet_probe()
>
> int
> usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
> {
> ....
> 	skb_queue_head_init (&dev->done);
> 	skb_queue_head_init(&dev->rxq_pause);
> 	dev->bh.func = usbnet_bh;
> 	dev->bh.data = (unsigned long) dev;
> 	INIT_WORK (&dev->kevent, usbnet_deferred_kevent);
> ....
>
>
> first, sometimes tasklet initialisation is performed directly, not via
> tasklet_init().
>
> second, that 't->count == 0' eq 'tasklet_init()' is assumed to be sort of
> a contract. so a simple kzalloc() works fine, and the patch breaks it.
>
Thats really bad that tasklet code is letting users call 
tasklet_schedule() even without any tasklet_init or DECLARE_TASKLET.

> a simple grep in drivers/net/
>
> _next$ git grep tasklet_sched drivers/net/ | awk '{print $1}' | uniq | wc -l
> 60
>
> _next$ git grep tasklet_init drivers/net/ | awk '{print $1}' | uniq | wc -l
> 52
>
> and I don't know how many call-sites outside of drivers/net/ do something
> like this.
>
There are more :-(. Thanks for helping out Sergey.

# git grep tasklet_sched . | awk '{print $1}' | uniq | wc -l
269
# git grep tasklet_init . | awk '{print $1}' | uniq | wc -l
240

Andrew,
I requested you to include this patch but now am not sure anymore.
Looks like there are almost 30 more users which are directly
tweaking 'tasklet_struct' fields and calling other APIs. Hunting them
and fixing them probably would be an exercise and also those changes
needs those changed drivers to be tested.

What do you suggest ? At least this patch needs to be dropped as of now
till we can have complete coverage for those bad users.

Regards,
Santosh