linux-kernel - Re: [PATCH v2] staging: pi433: cleanup tx

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] [day] [month] [year] [list]

Message-ID: <20b592c2-33b1-f2f6-875e-39402e5ab058@smarthome-wolf.de>
Date:   Thu, 19 Apr 2018 12:40:14 +0200
From:   Marcus Wolf <marcus.wolf@...rthome-wolf.de>
To:     Valentin Vidic <Valentin.Vidic@...Net.hr>
Cc:     Simon Sandström <simon@...anor.nu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        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 v2] staging: pi433: cleanup tx_fifo locking

Reviewed-by: Marcus Wolf <marcus.wolf@...f-entwicklungen.de>

Am 19.04.2018 um 12:25 schrieb Valentin Vidic:
> pi433_write requires locking due to multiple writers.  After acquiring
> the lock check if enough free space is available in the kfifo to write
> the whole message. This check should prevent partial writes to tx_fifo
> so kfifo_reset is not needed anymore.
> 
> pi433_tx_thread is the only reader so it does not require locking
> after kfifo_reset is removed.
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@...Net.hr>
> ---
> v2: print a warning if partial fifo write happens
> 
>  drivers/staging/pi433/pi433_if.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index d1e0ddbc79ce..2a05eff88469 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -87,7 +87,7 @@ struct pi433_device {
>  
>  	/* tx related values */
>  	STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
> -	struct mutex		tx_fifo_lock; // TODO: check, whether necessary or obsolete
> +	struct mutex		tx_fifo_lock; /* serialize userspace writers */
>  	struct task_struct	*tx_task_struct;
>  	wait_queue_head_t	tx_wait_queue;
>  	u8			free_in_fifo;
> @@ -589,19 +589,15 @@ pi433_tx_thread(void *data)
>  		 * - size of message
>  		 * - message
>  		 */
> -		mutex_lock(&device->tx_fifo_lock);
> -
>  		retval = kfifo_out(&device->tx_fifo, &tx_cfg, sizeof(tx_cfg));
>  		if (retval != sizeof(tx_cfg)) {
>  			dev_dbg(device->dev, "reading tx_cfg from fifo failed: got %d byte(s), expected %d", retval, (unsigned int)sizeof(tx_cfg));
> -			mutex_unlock(&device->tx_fifo_lock);
>  			continue;
>  		}
>  
>  		retval = kfifo_out(&device->tx_fifo, &size, sizeof(size_t));
>  		if (retval != sizeof(size_t)) {
>  			dev_dbg(device->dev, "reading msg size from fifo failed: got %d, expected %d", retval, (unsigned int)sizeof(size_t));
> -			mutex_unlock(&device->tx_fifo_lock);
>  			continue;
>  		}
>  
> @@ -634,7 +630,6 @@ pi433_tx_thread(void *data)
>  				   sizeof(device->buffer) - position);
>  		dev_dbg(device->dev,
>  			"read %d message byte(s) from fifo queue.", retval);
> -		mutex_unlock(&device->tx_fifo_lock);
>  
>  		/* if rx is active, we need to interrupt the waiting for
>  		 * incoming telegrams, to be able to send something.
> @@ -818,7 +813,7 @@ pi433_write(struct file *filp, const char __user *buf,
>  	struct pi433_instance	*instance;
>  	struct pi433_device	*device;
>  	int                     retval;
> -	unsigned int		copied;
> +	unsigned int		required, available, copied;
>  
>  	instance = filp->private_data;
>  	device = instance->device;
> @@ -833,6 +828,16 @@ pi433_write(struct file *filp, const char __user *buf,
>  	 * - message
>  	 */
>  	mutex_lock(&device->tx_fifo_lock);
> +
> +	required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
> +	available = kfifo_avail(&device->tx_fifo);
> +	if (required > available) {
> +		dev_dbg(device->dev, "write to fifo failed: %d bytes required but %d available",
> +			required, available);
> +		mutex_unlock(&device->tx_fifo_lock);
> +		return -EAGAIN;
> +	}
> +
>  	retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg,
>  			  sizeof(instance->tx_cfg));
>  	if (retval != sizeof(instance->tx_cfg))
> @@ -855,8 +860,8 @@ pi433_write(struct file *filp, const char __user *buf,
>  	return copied;
>  
>  abort:
> -	dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
> -	kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries
> +	dev_warn(device->dev,
> +		 "write to fifo failed, non recoverable: 0x%x", retval);
>  	mutex_unlock(&device->tx_fifo_lock);
>  	return -EAGAIN;
>  }
>