linux-kernel - Re: [PATCH] staging: pi433: add descriptions for mutex locks

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: <20180323221821.GP31333@gavran.carpriv.carnet.hr>
Date:   Fri, 23 Mar 2018 23:18:21 +0100
From:   Valentin Vidic <Valentin.Vidic@...Net.hr>
To:     Marcus Wolf <marcus.wolf@...rthome-wolf.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Simon Sandström <simon@...anor.nu>,
        Marcus Wolf <linux@...f-Entwicklungen.de>,
        Luca Söthe <luca@...l.me>,
        Marcin Ciupak <marcin.s.ciupak@...il.com>,
        Michael Panzlaff <michael.panzlaff@....de>,
        Derek Robson <robsonde@...il.com>, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: pi433: add descriptions for mutex locks

On Fri, Mar 23, 2018 at 07:00:27PM +0100, Valentin Vidic wrote:
> You are right, here is what kfifo.h says:
> 
> /*
>  * Note about locking : There is no locking required until only * one reader
>  * and one writer is using the fifo and no kfifo_reset() will be * called
>  *  kfifo_reset_out() can be safely used, until it will be only called
>  * in the reader thread.
>  *  For multiple writer and one reader there is only a need to lock the writer.
>  * And vice versa for only one writer and multiple reader there is only a need
>  * to lock the reader.
>  */
> 
> In the case of pi433 there is only one reader (pi433_tx_thread) and
> there is no need for a lock there. But the char device (pi433_write)
> might have multiple writers so we leave the mutex just in that function?

Ah, but there is a kfifo_reset call in pi433_write that requires a
mutex for both readers and writers:

"Usage of kfifo_reset is dangerous. It should be only called when the
fifo is exclusived locked or when it is secured that no other thread is
accessing the fifo."

Also kfifo_reset_out would probably not help here:

"The usage of kfifo_reset_out is safe until it will be only called from
the reader thread and there is only one concurrent reader. Otherwise it
is dangerous and must be handled in the same way as kfifo_reset."

But I have an idea to remove this kfifo_reset call in pi433_write
handling partial message writes:

  kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries

The writer could acquire the lock and than use kfifo_avail to check if
there is enough space to write the whole message.  What do you think?

-- 
Valentin