linux-kernel - Re: [PATCH 0/3] staging: r8188eu: use completions and clean rtw_cmd

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: <3115690.HXPuu0oz9h@localhost.localdomain>
Date:   Sat, 16 Oct 2021 19:54:51 +0200
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Phillip Potter <phil@...lpotter.co.uk>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/3] staging: r8188eu: use completions and clean rtw_cmd_thread()

On Saturday, October 16, 2021 5:27:40 PM CEST Phillip Potter wrote:
> On Sat, Oct 16, 2021 at 11:10:39AM +0200, Fabio M. De Francesco wrote:
> > This series replaces two semaphores with three completion variables
> > in rtw_cmd_thread(). Completions variables are better suited for the
> > purposes that are explained in detail in the commit messages of patches
> > 1/3 and 2/3. Furthermore, patch 3/3 removes a redundant 'if' statement
> > from that same rtw_cmd_thread().
> > 
> > Tested with ASUSTek Computer, Inc. Realtek 8188EUS [USB-N10 Nano]
> > 
> > Many thanks to Dan Carpenter <dan.carpenter@...cle.com> who helped with
> > his review of the RFC Patch.
> >
> > [...]
> >

Dear Phil,

> 
> Dear Fabio,
> 
> Built and tested on my USB-N10 Nano, working well. In terms of how
> you've split the patches out, I have no problem with it personally,

I guess that Dan will disagree with us :) Did you read his last message?

I hope that he has time to review these patches. He expressed some doubts 
about splitting the changes in two separate patches. As far as I know, since 
Dan is a very experienced engineer (I am not even graduated and everything I 
know of Computer Science is self-taught), I could have been wrong in doing 
this work the way I did.

> given that one semaphore was there for kthread start/stop and the other
> for queuing. Looks good to me anyway based on what I know of completion
> variables :-) I assume you've not made the waits killable or
> interruptible in patch 1 due to the fact they are specifically related
> to kthread start/stop?

Good question! :)

Let me explain how I chose to make one wait killable and the other 
uninterruptible.

As far as I know, waiters may spin or sleep while waiting to acquire a lock  
(see spinlocks or mutexes for instance) or to be awakened (completions and 
condition variables for instance).

These were the cases of sleeping waiters. Sleeping can be done in 
uninterruptible, interruptible / killable, and timed-out states.

Where I'm 100% sure that the code doesn't require / want to be interrupted 
for whatever reason I prefer to use uninterruptible variants (and so I did in 
1/3).

When I'm not sure of the requirement above, I prefer to avoid that the 
process or the entire system hangs while waiting to acquire a mutex or to be 
awakened by a complete() (and so on).

Conversely, using interruptible versions without proper checking of return 
codes and without proper managing of errors may lead to serious bugs.

Kernel threads (kthreads) are like user processes / threads and are scheduled 
the same way the former are. One noteworthy difference is that their mm 
pointer is NULL (they have not an userspace address spaces). However they are 
still threads that have a PID in userspace and they can be killed by root.

This is the output of the "ps -ef" command after "modprobe r8188eu":

localhost:~ # ps -ef | grep RTW
root      1726     2  0 19:06 ?        00:00:00 [RTW_CMD_THREAD]

Since the developers who wrote the original code thought that that thread 
must be interrupted I thought that restricting interruptions to kills was the 
wiser choice in 2/3. Conversely, I cannot see reasons to interrupt the core 
part of a driver, so I chose to use an uninterruptible version of 
wait_for_completion*() in the other parts of the code.

I warned you that I'm not an engineer, so please double check my argument :)

> Anyhow:
> 
> For whole series:
> Acked-by: Phillip Potter <phil@...lpotter.co.uk>

Thanks for you ack. I really appreciated it.

Best regards,

Fabio

> 
> Regards,
> Phil
>