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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4dba7594-2661-edd5-24ba-cffdf24021c5@smarthome-wolf.de>
Date:   Thu, 19 Apr 2018 11:33:59 +0200
From:   Marcus Wolf <marcus.wolf@...rthome-wolf.de>
To:     Valentin Vidic <Valentin.Vidic@...Net.hr>
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



Am 12.04.2018 um 20:46 schrieb Valentin Vidic:
> On Sun, Apr 08, 2018 at 05:22:46PM +0200, Marcus Wolf wrote:
>> Regarding your patch, I did not understand, why you did not remove
>> the mutex_lock in pi433_write. Wasn't it the goal to remove it?
> 
> Is it possible for more than one userspace program to open the pi433
> device and send messages?  In that case more than one pi433_write
> could be running and it needs to hold a mutex_lock before calling
> kfifo_in.

You are right. The write function is open for multiple userspace programs.
So mutex_lock schould remain.

>> Below find a proposal of pi433_write function, I wrote on base
>> of my outdated (!), private repo. It is not compiled and not tested.
>> Since there is no more handling in case of an error (as well in the
>> propsal as in your patch), I removed the error handling completely.
>> I only do a test to detect proplems while writing to the tx_fifo,
>> but (like in your patch) do nothing for solving, just printing a line.
>> If this unexpected situation will occur (most probably never),
>> the tx_fifo will be (and stay) out of sync until driver gets unloaded.
>> We have to decide, whether we can stay with that. Like written above,
>> I thinkt the benefits are great, the chance of such kind of error
>> very low.
>> What do you think?
> 
> Yes, if there is only one writer and it checks the available size,
> kfifo_in should not fail.  The only problem might be copy_from_user
> but perhaps that is also quite unlikely.  A workaround for that could
> be to copy the data into a temporary kernel buffer first and than
> start kfifo writes using only kernel memory.

For my feeling, that's a safe solution but most probably oversized.

>> It could be discussed, whether it is better to return EMSGSIZE or
>> EAGAIN on the first check. On the one hand, there is a problem with
>> the message size, on the other hand (if message isn't generally too
>> big) after a while, there should be some more space available in
>> fifo, so EAGAIN may be better choice.
> 
> EAGAIN does seem better unless the message is too big to ever fit
> in the kfifo.
> 
>> 	if (retval != required ) {
>> 		dev_dbg(device->dev, "write to fifo failed, reason unknown, non recoverable.");
>> 		return -EAGAIN;
>> 	}
> 
> Maybe this should be dev_warn or even dev_crit if the driver is not
> usable anymore when this happens?  The error message should than also
> be adjusted to EBADF or something similar.

>From my point of veiew, the warning is (in combination of the
size-check) the by far better solution then the fifo reset.

So all in all, I think, we should go with your proposal.

Unfortunaly still no time to setup my test environment with a current
version of the driver in order to give it a try :-(
Sorry,

Marcus