[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 30 Sep 2019 15:49:39 -0700
From: Kees Cook <keescook@...omium.org>
To: Romain Perier <romain.perier@...il.com>
Cc: kernel-hardening@...ts.openwall.com,
Romain Perier <romain.perier@...eris.fr>
Subject: Re: [PRE-REVIEW PATCH 13/16] tasklet: Pass tasklet_struct pointer to
callbacks unconditionally
On Sun, Sep 29, 2019 at 06:30:25PM +0200, Romain Perier wrote:
> From: Romain Perier <romain.perier@...eris.fr>
>
> Now that all tasklet callbacks are already taking their struct
> tasklet_struct pointer as the callback argument, just do this
> unconditionally and remove the .data field. It converts all
> runtime code, init functions and the DECLARE_TASKLET() macros
> to get rid of the .data argument. Moreover, all remaining use
> or assignment of the .data field are removed in a single shot.
>
> Signed-off-by: Romain Perier <romain.perier@...eris.fr>
> ---
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +-
> drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +-
> drivers/net/usb/usbnet.c | 1 -
> drivers/net/wireless/atmel/at76c50x-usb.c | 1 -
> drivers/net/wireless/intersil/hostap/hostap_hw.c | 2 +-
> drivers/net/wireless/realtek/rtlwifi/usb.c | 1 -
> drivers/net/wireless/zydas/zd1211rw/zd_usb.c | 1 -
> drivers/staging/most/dim2/dim2.c | 1 -
> drivers/usb/gadget/udc/snps_udc_core.c | 1 -
> include/linux/interrupt.h | 9 ++++-----
> kernel/softirq.c | 5 ++---
> net/atm/pppoatm.c | 1 -
> 12 files changed, 9 insertions(+), 18 deletions(-)
Similar to the other suggestions, I would split out the individual
changes that that touch the .data assignments. This makes those
"unusual" cases easier to review.
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 82630db0394b..357d14ae32d0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1057,7 +1057,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
> if (tasklet_trylock(t)) {
> /* Must wait for any GPU reset in progress. */
> if (__tasklet_is_enabled(t))
> - t->func(t->data);
> + t->func(t);
> tasklet_unlock(t);
> }
> local_bh_enable();
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index d630dbc953ad..99f39047e49f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2616,7 +2616,7 @@ static void execlists_reset_finish(struct intel_engine_cs *engine)
> */
> GEM_BUG_ON(!reset_in_progress(execlists));
> if (!RB_EMPTY_ROOT(&execlists->queue.rb_root))
> - execlists->tasklet.func(execlists->tasklet.data);
> + execlists->tasklet.func(&execlists->tasklet);
>
> if (__tasklet_enable(&execlists->tasklet))
> /* And kick in case we missed a new request submission. */
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 1c4817c05d3c..14cedee27f2f 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1693,7 +1693,6 @@ 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 = (TASKLET_FUNC_TYPE)usbnet_bh;
> - dev->bh.data = (TASKLET_DATA_TYPE)&dev->delay;
> INIT_WORK (&dev->kevent, usbnet_deferred_kevent);
> init_usb_anchor(&dev->deferred);
> timer_setup(&dev->delay, usbnet_bh, 0);
> diff --git a/drivers/net/wireless/atmel/at76c50x-usb.c b/drivers/net/wireless/atmel/at76c50x-usb.c
> index 3a66538a0853..fd796a32c71d 100644
> --- a/drivers/net/wireless/atmel/at76c50x-usb.c
> +++ b/drivers/net/wireless/atmel/at76c50x-usb.c
> @@ -1199,7 +1199,6 @@ static void at76_rx_callback(struct urb *urb)
> {
> struct at76_priv *priv = urb->context;
>
> - priv->rx_tasklet.data = (unsigned long)urb;
Some of these looks very strange -- how are the urb and tasklet
associated, etc?
> tasklet_schedule(&priv->rx_tasklet);
> }
>
> diff --git a/drivers/net/wireless/intersil/hostap/hostap_hw.c b/drivers/net/wireless/intersil/hostap/hostap_hw.c
> index 187095e13294..ae656563a82d 100644
> --- a/drivers/net/wireless/intersil/hostap/hostap_hw.c
> +++ b/drivers/net/wireless/intersil/hostap/hostap_hw.c
> @@ -3183,7 +3183,7 @@ prism2_init_local_data(struct prism2_helper_functions *funcs, int card_idx,
> /* Initialize tasklets for handling hardware IRQ related operations
> * outside hw IRQ handler */
> #define HOSTAP_TASKLET_INIT(q, f, d) \
> -do { memset((q), 0, sizeof(*(q))); (q)->func = (TASKLET_FUNC_TYPE)(f); (q)->data = (TASKLET_DATA_TYPE)(q); } \
> +do { memset((q), 0, sizeof(*(q))); (q)->func = (TASKLET_FUNC_TYPE)(f); } \
> while (0)
> HOSTAP_TASKLET_INIT(&local->bap_tasklet, hostap_bap_tasklet,
> (unsigned long) local);
> diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> index aae5c5aa1769..7e9bdbd2a6f4 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> @@ -311,7 +311,6 @@ static int _rtl_usb_init_rx(struct ieee80211_hw *hw)
>
> skb_queue_head_init(&rtlusb->rx_queue);
> rtlusb->rx_work_tasklet.func = (TASKLET_FUNC_TYPE)_rtl_rx_work;
> - rtlusb->rx_work_tasklet.data = (TASKLET_DATA_TYPE)&rtlusb->rx_work_tasklet;
>
> return 0;
> }
> diff --git a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
> index 0ebbd727a452..4275549f4d98 100644
> --- a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
> +++ b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
> @@ -1181,7 +1181,6 @@ static inline void init_usb_rx(struct zd_usb *usb)
> ZD_ASSERT(rx->fragment_length == 0);
> INIT_DELAYED_WORK(&rx->idle_work, zd_rx_idle_timer_handler);
> rx->reset_timer_tasklet.func = (TASKLET_FUNC_TYPE)zd_usb_reset_rx_idle_timer_tasklet;
> - rx->reset_timer_tasklet.data = (TASKLET_DATA_TYPE)&rx->reset_timer_tasklet;
These look like open-coded tasklet initializations?
--
Kees Cook
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
