| Message ID | 20200716030847.1564131-1-keescook@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Modernize tasklet callback API | expand |
On Wed, Jul 15, 2020 at 08:08:44PM -0700, Kees Cook wrote: > Hi, > > This is the infrastructure changes to prepare the tasklet API for > conversion to passing the tasklet struct as the callback argument instead > of an arbitrary unsigned long. The first patch details why this is useful > (it's the same rationale as the timer_struct changes from a bit ago: > less abuse during memory corruption attacks, more in line with existing > ways of doing things in the kernel, save a little space in struct, > etc). Notably, the existing tasklet API use is much less messy, so there > is less to clean up. I would _MUCH_ rather see tasklets go the way of the dodo, esp. given that: > drivers/input/keyboard/omap-keypad.c | 2 +- > drivers/input/serio/hil_mlc.c | 2 +- > drivers/net/wan/farsync.c | 4 +-- > drivers/s390/crypto/ap_bus.c | 2 +- > drivers/staging/most/dim2/dim2.c | 2 +- > drivers/staging/octeon/ethernet-tx.c | 2 +- > drivers/tty/vt/keyboard.c | 2 +- > drivers/usb/gadget/udc/snps_udc_core.c | 6 ++--- > drivers/usb/host/fhci-sched.c | 2 +- > include/linux/interrupt.h | 37 ++++++++++++++++++++++---- > kernel/backtracetest.c | 2 +- > kernel/debug/debug_core.c | 2 +- > kernel/irq/resend.c | 2 +- > kernel/softirq.c | 18 ++++++++++++- > net/atm/pppoatm.c | 2 +- > net/iucv/iucv.c | 2 +- > sound/drivers/pcsp/pcsp_lib.c | 2 +- > 17 files changed, 66 insertions(+), 25 deletions(-) there appear to be hardly any users left.. Can't we stage an extinction event here instead?
On 2020-07-16 09:57:18 [+0200], Peter Zijlstra wrote: > > there appear to be hardly any users left.. Can't we stage an extinction > event here instead? Most of the time the tasklet is scheduled from an interrupt handler. So we could get rid of these tasklets by using threaded IRQs. Sebastian
On Thu, Jul 16, 2020 at 09:57:18AM +0200, Peter Zijlstra wrote: > On Wed, Jul 15, 2020 at 08:08:44PM -0700, Kees Cook wrote: > > Hi, > > > > This is the infrastructure changes to prepare the tasklet API for > > conversion to passing the tasklet struct as the callback argument instead > > of an arbitrary unsigned long. The first patch details why this is useful > > (it's the same rationale as the timer_struct changes from a bit ago: > > less abuse during memory corruption attacks, more in line with existing > > ways of doing things in the kernel, save a little space in struct, > > etc). Notably, the existing tasklet API use is much less messy, so there > > is less to clean up. > > I would _MUCH_ rather see tasklets go the way of the dodo, esp. given > that: > > > drivers/input/keyboard/omap-keypad.c | 2 +- > > drivers/input/serio/hil_mlc.c | 2 +- > > drivers/net/wan/farsync.c | 4 +-- > > drivers/s390/crypto/ap_bus.c | 2 +- > > drivers/staging/most/dim2/dim2.c | 2 +- > > drivers/staging/octeon/ethernet-tx.c | 2 +- > > drivers/tty/vt/keyboard.c | 2 +- > > drivers/usb/gadget/udc/snps_udc_core.c | 6 ++--- > > drivers/usb/host/fhci-sched.c | 2 +- > > include/linux/interrupt.h | 37 ++++++++++++++++++++++---- > > kernel/backtracetest.c | 2 +- > > kernel/debug/debug_core.c | 2 +- > > kernel/irq/resend.c | 2 +- > > kernel/softirq.c | 18 ++++++++++++- > > net/atm/pppoatm.c | 2 +- > > net/iucv/iucv.c | 2 +- > > sound/drivers/pcsp/pcsp_lib.c | 2 +- > > 17 files changed, 66 insertions(+), 25 deletions(-) > > there appear to be hardly any users left.. Can't we stage an extinction > event here instead? Oh, I wish, but no. That's just the ones using DECLARE_TASKLET. There are hundred(s?) more (see the referenced tree).
On Thu, Jul 16, 2020 at 10:15:38AM +0200, Sebastian Andrzej Siewior wrote: > On 2020-07-16 09:57:18 [+0200], Peter Zijlstra wrote: > > > > there appear to be hardly any users left.. Can't we stage an extinction > > event here instead? > > Most of the time the tasklet is scheduled from an interrupt handler. So > we could get rid of these tasklets by using threaded IRQs. Perhaps I can add a comment above the tasklet API area in interrupt.h?
On Thu, Jul 16, 2020 at 12:14 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Jul 16, 2020 at 09:57:18AM +0200, Peter Zijlstra wrote: > > On Wed, Jul 15, 2020 at 08:08:44PM -0700, Kees Cook wrote: > > > Hi, > > > > > > This is the infrastructure changes to prepare the tasklet API for > > > conversion to passing the tasklet struct as the callback argument instead > > > of an arbitrary unsigned long. The first patch details why this is useful > > > (it's the same rationale as the timer_struct changes from a bit ago: > > > less abuse during memory corruption attacks, more in line with existing > > > ways of doing things in the kernel, save a little space in struct, > > > etc). Notably, the existing tasklet API use is much less messy, so there > > > is less to clean up. > > > > I would _MUCH_ rather see tasklets go the way of the dodo, esp. given > > that: > > > > > drivers/input/keyboard/omap-keypad.c | 2 +- > > > drivers/input/serio/hil_mlc.c | 2 +- > > > drivers/net/wan/farsync.c | 4 +-- > > > drivers/s390/crypto/ap_bus.c | 2 +- > > > drivers/staging/most/dim2/dim2.c | 2 +- > > > drivers/staging/octeon/ethernet-tx.c | 2 +- > > > drivers/tty/vt/keyboard.c | 2 +- > > > drivers/usb/gadget/udc/snps_udc_core.c | 6 ++--- > > > drivers/usb/host/fhci-sched.c | 2 +- > > > include/linux/interrupt.h | 37 ++++++++++++++++++++++---- > > > kernel/backtracetest.c | 2 +- > > > kernel/debug/debug_core.c | 2 +- > > > kernel/irq/resend.c | 2 +- > > > kernel/softirq.c | 18 ++++++++++++- > > > net/atm/pppoatm.c | 2 +- > > > net/iucv/iucv.c | 2 +- > > > sound/drivers/pcsp/pcsp_lib.c | 2 +- > > > 17 files changed, 66 insertions(+), 25 deletions(-) > > > > there appear to be hardly any users left.. Can't we stage an extinction > > event here instead? > > Oh, I wish, but no. That's just the ones using DECLARE_TASKLET. There > are hundred(s?) more (see the referenced tree). Still, do we really need tasklets? Can we substitute timers executing immediately in their place? Thanks.
On Thu, Jul 16, 2020 at 01:48:20PM -0700, Dmitry Torokhov wrote: > On Thu, Jul 16, 2020 at 12:14 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Thu, Jul 16, 2020 at 09:57:18AM +0200, Peter Zijlstra wrote: > > > On Wed, Jul 15, 2020 at 08:08:44PM -0700, Kees Cook wrote: > > > > Hi, > > > > > > > > This is the infrastructure changes to prepare the tasklet API for > > > > conversion to passing the tasklet struct as the callback argument instead > > > > of an arbitrary unsigned long. The first patch details why this is useful > > > > (it's the same rationale as the timer_struct changes from a bit ago: > > > > less abuse during memory corruption attacks, more in line with existing > > > > ways of doing things in the kernel, save a little space in struct, > > > > etc). Notably, the existing tasklet API use is much less messy, so there > > > > is less to clean up. > > > > > > I would _MUCH_ rather see tasklets go the way of the dodo, esp. given > > > that: > > > > > > > drivers/input/keyboard/omap-keypad.c | 2 +- > > > > drivers/input/serio/hil_mlc.c | 2 +- > > > > drivers/net/wan/farsync.c | 4 +-- > > > > drivers/s390/crypto/ap_bus.c | 2 +- > > > > drivers/staging/most/dim2/dim2.c | 2 +- > > > > drivers/staging/octeon/ethernet-tx.c | 2 +- > > > > drivers/tty/vt/keyboard.c | 2 +- > > > > drivers/usb/gadget/udc/snps_udc_core.c | 6 ++--- > > > > drivers/usb/host/fhci-sched.c | 2 +- > > > > include/linux/interrupt.h | 37 ++++++++++++++++++++++---- > > > > kernel/backtracetest.c | 2 +- > > > > kernel/debug/debug_core.c | 2 +- > > > > kernel/irq/resend.c | 2 +- > > > > kernel/softirq.c | 18 ++++++++++++- > > > > net/atm/pppoatm.c | 2 +- > > > > net/iucv/iucv.c | 2 +- > > > > sound/drivers/pcsp/pcsp_lib.c | 2 +- > > > > 17 files changed, 66 insertions(+), 25 deletions(-) > > > > > > there appear to be hardly any users left.. Can't we stage an extinction > > > event here instead? > > > > Oh, I wish, but no. That's just the ones using DECLARE_TASKLET. There > > are hundred(s?) more (see the referenced tree). > > Still, do we really need tasklets? Can we substitute timers executing > immediately in their place? If there is a direct replacement, then sure, I'd be happy to do whatever, however it does not look mechanical to me. If there is a mechanical way that will convert these two directories (as an example of various complexities): drivers/crypto/ccp/ drivers/gpu/drm/i915/gt/ then let's get it documented. But if not, let's write up a paragraph for the deprecated.rst, mark it as deprecated in comments, and modernize the API (which is a mostly mechanical change) to avoid it being a problem for CFI, for memory corruption, and heap space, etc.
Kees, Kees Cook <keescook@chromium.org> writes: > This is the infrastructure changes to prepare the tasklet API for > conversion to passing the tasklet struct as the callback argument instead > of an arbitrary unsigned long. The first patch details why this is useful > (it's the same rationale as the timer_struct changes from a bit ago: > less abuse during memory corruption attacks, more in line with existing > ways of doing things in the kernel, save a little space in struct, > etc). Notably, the existing tasklet API use is much less messy, so there > is less to clean up. > > It's not clear to me which tree this should go through... Greg since it > starts with a USB clean-up, -tip for timer or interrupt, or if I should > just carry it. I'm open to suggestions, but if I don't hear otherwise, > I'll just carry it. > > My goal is to have this merged for v5.9-rc1 so that during the v5.10 > development cycle the new API will be available. The entire tree of > changes is here[1] currently, but to split it up by maintainer the > infrastructure changes need to be landed first. > > Review and Acks appreciated! :) I'd rather see tasklets vanish from the planet completely, but that's going to be a daring feat. So, grudgingly: Acked-by: Thomas Gleixner <tglx@linutronix.de>
[heavily trimmed CC list because I think lkml is ignoring this thread...] On Thu, Jul 30, 2020 at 09:03:55AM +0200, Thomas Gleixner wrote: > Kees, > > Kees Cook <keescook@chromium.org> writes: > > This is the infrastructure changes to prepare the tasklet API for > > conversion to passing the tasklet struct as the callback argument instead > > of an arbitrary unsigned long. The first patch details why this is useful > > (it's the same rationale as the timer_struct changes from a bit ago: > > less abuse during memory corruption attacks, more in line with existing > > ways of doing things in the kernel, save a little space in struct, > > etc). Notably, the existing tasklet API use is much less messy, so there > > is less to clean up. > > > > It's not clear to me which tree this should go through... Greg since it > > starts with a USB clean-up, -tip for timer or interrupt, or if I should > > just carry it. I'm open to suggestions, but if I don't hear otherwise, > > I'll just carry it. > > > > My goal is to have this merged for v5.9-rc1 so that during the v5.10 > > development cycle the new API will be available. The entire tree of > > changes is here[1] currently, but to split it up by maintainer the > > infrastructure changes need to be landed first. > > > > Review and Acks appreciated! :) > > I'd rather see tasklets vanish from the planet completely, but that's > going to be a daring feat. So, grudgingly: Understood! I will update the comments near the tasklet API. > Acked-by: Thomas Gleixner <tglx@linutronix.de> Thanks!
Kees, > > [heavily trimmed CC list because I think lkml is ignoring this > thread...] > > On Thu, Jul 30, 2020 at 09:03:55AM +0200, Thomas Gleixner wrote: > > Kees, > > > > Kees Cook <keescook@chromium.org> writes: > > > This is the infrastructure changes to prepare the tasklet API for > > > conversion to passing the tasklet struct as the callback argument instead > > > of an arbitrary unsigned long. The first patch details why this is useful > > > (it's the same rationale as the timer_struct changes from a bit ago: > > > less abuse during memory corruption attacks, more in line with existing > > > ways of doing things in the kernel, save a little space in struct, > > > etc). Notably, the existing tasklet API use is much less messy, so there > > > is less to clean up. > > > > > > It's not clear to me which tree this should go through... Greg since it > > > starts with a USB clean-up, -tip for timer or interrupt, or if I should > > > just carry it. I'm open to suggestions, but if I don't hear otherwise, > > > I'll just carry it. > > > > > > My goal is to have this merged for v5.9-rc1 so that during the v5.10 > > > development cycle the new API will be available. The entire tree of > > > changes is here[1] currently, but to split it up by maintainer the > > > infrastructure changes need to be landed first. > > > > > > Review and Acks appreciated! :) > > > > I'd rather see tasklets vanish from the planet completely, but that's > > going to be a daring feat. So, grudgingly: > > Understood! I will update the comments near the tasklet API. > > > Acked-by: Thomas Gleixner <tglx@linutronix.de> > Here's the series re-based on top of 5.8 https://github.com/allenpais/tasklets/tree/V3 Let me know how you would want these to be reviewed. Also, I was thinking if removing tasklets completely could be a task on KSPP wiki. If yes, I did like to take ownership of that task. I have a couple of ideas in mind, which could be discussed in a separate email. Thanks.
Kees, > > > > Here's the series re-based on top of 5.8 > https://github.com/allenpais/tasklets/tree/V3 > > Let me know how you would want these to be reviewed. > I see the first set of infrastructure patches for tasklets have landed in Linus's tree. Good time to send out the ~200 patches? - Allen
On Mon, Aug 03, 2020 at 02:16:15PM +0530, Allen wrote: > Here's the series re-based on top of 5.8 > https://github.com/allenpais/tasklets/tree/V3 Great! > Let me know how you would want these to be reviewed. Was a Coccinelle script used for any of these conversions? I wonder if it'd be easier to do a single treewide patch for the more mechanical changes. And, actually, I still think the "prepare" patches should just be collapsed into the actual "covert" patches -- there are only a few. After those, yeah, I think getting these sent to their respective maintainers is the next step. > Also, I was thinking if removing tasklets completely could be a task > on KSPP wiki. If yes, I did like to take ownership of that task. I have a > couple of ideas in mind, which could be discussed in a separate email. Sure! I will add it to the tracker. Here's for the refactoring: https://github.com/KSPP/linux/issues/30 and here's for the removal: https://github.com/KSPP/linux/issues/94 if you can added details/examples of how they should be removed, that'd help other folks too, if they wanted to jump in. :) -Kees
On Tue, 11 Aug 2020 23:33:13 +0200, Kees Cook wrote: > > On Mon, Aug 03, 2020 at 02:16:15PM +0530, Allen wrote: > > Here's the series re-based on top of 5.8 > > https://github.com/allenpais/tasklets/tree/V3 > > Great! > > > Let me know how you would want these to be reviewed. > > Was a Coccinelle script used for any of these conversions? I wonder if > it'd be easier to do a single treewide patch for the more mechanical > changes. > > And, actually, I still think the "prepare" patches should just be > collapsed into the actual "covert" patches -- there are only a few. > > After those, yeah, I think getting these sent to their respective > maintainers is the next step. > > > Also, I was thinking if removing tasklets completely could be a task > > on KSPP wiki. If yes, I did like to take ownership of that task. I have a > > couple of ideas in mind, which could be discussed in a separate email. > > Sure! I will add it to the tracker. Here's for the refactoring: > https://github.com/KSPP/linux/issues/30 > > and here's for the removal: > https://github.com/KSPP/linux/issues/94 > > if you can added details/examples of how they should be removed, that'd > help other folks too, if they wanted to jump in. :) I have a patch set to convert the remaining tasklet usage in sound drivers to either the threaded IRQ or the work, but it wasn't submitted / merged for 5.8 due to the obvious conflict with your API changes. Each conversion is rather simple, but it's always a question of the nature of each tasklet usage which alternative is the best fit. FWIW, the current version is found in test/kill-tasklet branch of sound git tree git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git thanks, Takashi
> > I have a patch set to convert the remaining tasklet usage in sound > drivers to either the threaded IRQ or the work, but it wasn't > submitted / merged for 5.8 due to the obvious conflict with your API > changes. > Each conversion is rather simple, but it's always a question of the > nature of each tasklet usage which alternative is the best fit. > > FWIW, the current version is found in test/kill-tasklet branch of > sound git tree > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git Great. Currently my tree has these converted to use the new tasklet_setup() api. I will add these to my threaded IRQ/work tree (which is still wip). Thanks.
Kees, > Was a Coccinelle script used for any of these conversions? I wonder if > it'd be easier to do a single treewide patch for the more mechanical > changes. No, I should have written one. Will do it. > And, actually, I still think the "prepare" patches should just be > collapsed into the actual "covert" patches -- there are only a few. Okay. It's been done and pushed to: https://github.com/allenpais/tasklets/tree/V4 > After those, yeah, I think getting these sent to their respective > maintainers is the next step. Please look at the above branch, if it looks fine, let me know if I can add your ACK on the patches. > > Sure! I will add it to the tracker. Here's for the refactoring: > https://github.com/KSPP/linux/issues/30 > > and here's for the removal: > https://github.com/KSPP/linux/issues/94 > > if you can added details/examples of how they should be removed, that'd > help other folks too, if they wanted to jump in. :) Sure, Thank you. - Allen