Kernel-hardening archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Romain Perier <romain.perier@gmail.com>
Cc: kernel-hardening@lists.openwall.com,
	Romain Perier <romain.perier@viveris.fr>
Subject: Re: [PRE-REVIEW PATCH 13/16] tasklet: Pass tasklet_struct pointer to callbacks unconditionally
Date: Mon, 30 Sep 2019 15:49:39 -0700
Message-ID: <201909301547.8DC227B@keescook> (raw)
In-Reply-To: <20190929163028.9665-14-romain.perier@gmail.com>

On Sun, Sep 29, 2019 at 06:30:25PM +0200, Romain Perier wrote:
> From: Romain Perier <romain.perier@viveris.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@viveris.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

  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-29 16:30 [PRE-REVIEW PATCH 00/16] Modernize the tasklet API Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 01/16] tasklet: Prepare to change tasklet callback argument type Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 02/16] crypto: ccp - Prepare to use the new tasklet API Romain Perier
2019-09-30 22:35   ` Kees Cook
2019-09-29 16:30 ` [PRE-REVIEW PATCH 03/16] mmc: renesas_sdhi: " Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 04/16] net: liquidio: " Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 05/16] chelsio: " Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 06/16] net: mvpp2: " Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 07/16] qed: " Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 08/16] isdn: " Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 09/16] scsi: pm8001: " Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 10/16] scsi: pmcraid: " Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 11/16] treewide: Globally replace tasklet_init() by tasklet_setup() Romain Perier
2019-09-30 22:46   ` Kees Cook
2019-10-01 17:18     ` Romain Perier
2019-10-10 22:30       ` Kees Cook
2019-09-29 16:30 ` [PRE-REVIEW PATCH 12/16] tasklet: Pass tasklet_struct pointer as .data in DECLARE_TASKLET Romain Perier
2019-09-30 22:44   ` Kees Cook
2019-09-29 16:30 ` [PRE-REVIEW PATCH 13/16] tasklet: Pass tasklet_struct pointer to callbacks unconditionally Romain Perier
2019-09-30 22:49   ` Kees Cook [this message]
2019-09-29 16:30 ` [PRE-REVIEW PATCH 14/16] tasklet: Remove the data argument from DECLARE_TASKLET() macros Romain Perier
2019-09-30 22:50   ` Kees Cook
2019-09-29 16:30 ` [PRE-REVIEW PATCH 15/16] tasklet: convert callbacks prototype for using struct tasklet_struct * arguments Romain Perier
2019-09-29 16:30 ` [PRE-REVIEW PATCH 16/16] tasklet: Add the new initialization function permanently Romain Perier
2019-09-30 22:52   ` Kees Cook
2019-10-01 17:34     ` Romain Perier
2019-09-30 23:06 ` [PRE-REVIEW PATCH 00/16] Modernize the tasklet API Kees Cook
2019-10-01 17:47   ` Romain Perier
2019-10-10 22:34     ` Kees Cook
2019-10-30  8:20       ` Allen
2019-11-07  7:29         ` Romain Perier
2019-11-07 21:22           ` Kees Cook

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201909301547.8DC227B@keescook \
    --to=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=romain.perier@gmail.com \
    --cc=romain.perier@viveris.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Kernel-hardening archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernel-hardening/0 kernel-hardening/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kernel-hardening kernel-hardening/ https://lore.kernel.org/kernel-hardening \
		kernel-hardening@lists.openwall.com
	public-inbox-index kernel-hardening

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.openwall.lists.kernel-hardening


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git