linux-kernel - Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores

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: <20211015113715.GR8429@kadam>
Date:   Fri, 15 Oct 2021 14:37:15 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] staging: r8188eu: Use completions instead of
 semaphores

On Fri, Oct 15, 2021 at 01:02:38PM +0200, Fabio M. De Francesco wrote:
> rtw_cmd_thread() "up(s)" a semaphore twice, first to notify callers when
> its execution is started and then to notify when it is about to end.
> 
> It makes the same semaphore go "up" twice in the same thread. This
> construct makes Smatch to warn of duplicate "up(s)".
> 
> This thread uses interruptible semaphores where instead completions are
> more suitable. For this purpose it calls an helper (_rtw_down_sema())
> that returns values that are never checked. It may lead to bugs.
> 
> To address the above-mentioned issues, use two completions variables
> instead of semaphores. Use the uninterruptible versions of
> wake_for_completion*() because the interruptible / killable versions are
> not necessary.
> 
> Tested with "ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano]".
> 
> This is an RFC patch because I'm not sure that changing this code
> from using semaphores to using completions variables is actually required.
> After all, the code was working properly with semaphores and, at the same
> time, I'm not sure if the Smatch warning about duplicate "up(s)" should
> actually be addressed.
> 
> I'm waiting for Maintainers and other Reviewers to say if this patch is
> actually needed and, if so, also for suggestions about how to improve
> it. In particular I'm interested to know what they think of using the
> uninterruptible version of wait_for_completion*().
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>

This is basically what Arnd did to rtl8723bs in commit:

commit 09a8ea34cf431bfb77159197e46753d101c528c5
Author: Arnd Bergmann <arnd@...db.de>
Date:   Mon Dec 10 22:40:30 2018 +0100

    staging: rtl8723bs: change semaphores to completions

But there are some differences.  His patch is a little bit cleaner
because it gets rid of "pcmdpriv->cmd_queue_sema".  Could you basically
just ports Arnd's patch for this driver?

His patch goes quite a bit further as well, and change some other
semaphors but we could do it piece meal and just change the
rtw_cmd_thread() related ones.

regards,
dan carpenter