linux-kernel - Re: [PATCH] axis-fifo: Remove hardware resets for user errors

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: <1a34379e-4c41-40ec-99f9-87342c33b45c@stanley.mountain>
Date: Tue, 22 Apr 2025 12:56:08 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Gabriel Shahrouzi <gshahrouzi@...il.com>
Cc: gregkh@...uxfoundation.org, jacobsfeder@...il.com,
	linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev,
	sergio.paracuellos@...il.com, skhan@...uxfoundation.org,
	linux-kernel-mentees@...ts.linux.dev, stable@...r.kernel.org
Subject: Re: [PATCH] axis-fifo:  Remove hardware resets for user errors

On Fri, Apr 18, 2025 at 08:43:06PM -0400, Gabriel Shahrouzi wrote:
> The axis-fifo driver performs a full hardware reset (via
> reset_ip_core()) in several error paths within the read and write
> functions. This reset flushes both TX and RX FIFOs and resets the
> AXI-Stream links.
> 
> Allow the user to handle the error without causing hardware disruption
> or data loss in other FIFO paths.
> 

I agree with the sentiment behind these changes, but they are basically
impossible to review.  The reset_ip_core() does some magic stuff in the
firmware and I don't have access to that.  How are you testing these
changes?

> Fixes: 4a965c5f89de ("staging: add driver for Xilinx AXI-Stream FIFO v4.1 IP core")
> Cc: stable@...r.kernel.org
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@...il.com>
> ---
>  drivers/staging/axis-fifo/axis-fifo.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index 7540c20090c78..76db29e4d2828 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -393,16 +393,14 @@ static ssize_t axis_fifo_read(struct file *f, char __user *buf,
>  
>  	bytes_available = ioread32(fifo->base_addr + XLLF_RLR_OFFSET);
>  	if (!bytes_available) {
> -		dev_err(fifo->dt_device, "received a packet of length 0 - fifo core will be reset\n");
> -		reset_ip_core(fifo);
> +		dev_err(fifo->dt_device, "received a packet of length 0\n");
>  		ret = -EIO;
>  		goto end_unlock;
>  	}
>  
>  	if (bytes_available > len) {
> -		dev_err(fifo->dt_device, "user read buffer too small (available bytes=%zu user buffer bytes=%zu) - fifo core will be reset\n",
> +		dev_err(fifo->dt_device, "user read buffer too small (available bytes=%zu user buffer bytes=%zu)\n",
>  			bytes_available, len);
> -		reset_ip_core(fifo);
>  		ret = -EINVAL;
>  		goto end_unlock;
>  	}
> @@ -411,8 +409,7 @@ static ssize_t axis_fifo_read(struct file *f, char __user *buf,
>  		/* this probably can't happen unless IP
>  		 * registers were previously mishandled
>  		 */
> -		dev_err(fifo->dt_device, "received a packet that isn't word-aligned - fifo core will be reset\n");
> -		reset_ip_core(fifo);
> +		dev_err(fifo->dt_device, "received a packet that isn't word-aligned\n");


The commit message talks about "user errors" but these aren't user
errors so far as I can see.

>  		ret = -EIO;
>  		goto end_unlock;
>  	}
> @@ -433,7 +430,6 @@ static ssize_t axis_fifo_read(struct file *f, char __user *buf,
>  
>  		if (copy_to_user(buf + copied * sizeof(u32), tmp_buf,
>  				 copy * sizeof(u32))) {
> -			reset_ip_core(fifo);

Yes.  Absolutely.  Delete this.

>  			ret = -EFAULT;
>  			goto end_unlock;
>  		}
> @@ -542,7 +538,6 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
>  
>  		if (copy_from_user(tmp_buf, buf + copied * sizeof(u32),
>  				   copy * sizeof(u32))) {
> -			reset_ip_core(fifo);

Same.  Delete.

This type of code is often written for a reason.  Potentially as a hack
to paper over a real bug.  And then people get carried away adding resets
all over the place.  It's fine to delete the last two calls but I would
be very careful to delete the others.  Even though the patch might be
correct it needs to be tested very carefully.

regards,
dan carpenter

>  			ret = -EFAULT;
>  			goto end_unlock;
>  		}
> -- 
> 2.43.0
>