linux-kernel - Re: [PATCH 5/7] staging: wilc1000: Replace semaphore close_exit

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]

Message-ID: <3987173.lIusvGA2iF@wuerfel>
Date:	Mon, 13 Jun 2016 15:42:08 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Binoy Jayan <binoy.jayan@...aro.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Johnny Kim <johnny.kim@...el.com>,
	Austin Shin <austin.shin@...el.com>,
	Chris Park <chris.park@...el.com>,
	Tony Cho <tony.cho@...el.com>, Glen Lee <glen.lee@...el.com>,
	Leo Kim <leo.kim@...el.com>, devel@...verdev.osuosl.org,
	linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] staging: wilc1000: Replace semaphore close_exit_sync with completion

On Monday, June 13, 2016 4:07:37 PM CEST Binoy Jayan wrote:
> @@ -31,7 +31,7 @@ static struct notifier_block g_dev_notifier = {
> 
>         .notifier_call = dev_state_ev_handler
>  
>  };
> 
> -static struct semaphore close_exit_sync;
> +static struct completion close_exit_sync;
> 
>  static int wlan_deinit_locks(struct net_device *dev);
>  static void wlan_deinitialize_threads(struct net_device *dev);
> @@ -1088,7 +1088,7 @@ int wilc_mac_close(struct net_device *ndev)
>                 WILC_WFI_deinit_mon_interface();
>         }
>  
> -       up(&close_exit_sync);
> +       complete(&close_exit_sync);
>         vif->mac_opened = 0;
>  
>         return 0;
> @@ -1232,7 +1232,8 @@ void wilc_netdev_cleanup(struct wilc *wilc)
>         }
>  
>         if (wilc && (wilc->vif[0]->ndev || wilc->vif[1]->ndev)) {
> -               wilc_lock_timeout(wilc, &close_exit_sync, 5 * 1000);
> +               wait_for_completion_timeout(&close_exit_sync,
> +                                               msecs_to_jiffies(5000));
>  
>                 for (i = 0; i < NUM_CONCURRENT_IFC; i++)
>                         if (wilc->vif[i]->ndev)
> @@ -1258,7 +1259,7 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
>         struct net_device *ndev;
>         struct wilc *wl;
>  
> -       sema_init(&close_exit_sync, 0);
> +       init_completion(&close_exit_sync);
>  
>         wl = kzalloc(sizeof(*wl), GFP_KERNEL);
>         if (!wl)

Here the original code seems wrong. Your patch doesn't make it worse, but
it also doesn't make it better.

What I see here is:

- There is a global semaphore for what should be per-device if anything

- we call up() or complete() every time we close one of the subdevices,
  but we do nothing when we open them, so the count will just go up
  over time as the device is being used.

- if the device is never used, the semaphore is locked and we end up
  having to wait for the timeout here, for no reason at all.

- if the timeout happens, this has no other consequences than running
  the following code later, we don't even warn about this.

- I don't see any reason why we even need a semaphore or completion here,
  it only blocks (or delays) the unregistering of the netdev, which
  we should always be able to unregister.

To conclude, I think we can just remove this semaphore without a replacement
(but with my findings above). Maybe the owners of the driver can shed some
more light on this question.

	Arnd