Re: [PATCH net v3 2/3] net: ethernet: oa_tc6: Improvement in buffer overflow handling

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: ethernet: oa_tc6: Improvement in buffer overflow handling

This patch teaches the oa_tc6 rx path to detect when tail + length would
exceed skb->end, abandon the current rx_skb, and resume on the next
data chunk carrying a start_valid bit. It also propagates -EAGAIN as a
recoverable error through the chunk/footer processing and the outer
SPI transfer loop.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 5b24cce4f9b5..a6b8762f6052 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -931,8 +966,7 @@ static int oa_tc6_prcs_rx_chunk_payload(struct oa_tc6 *tc6, u8 *data,
>  	/* Process the chunk with only rx frame end */
>  	if (end_valid && !start_valid) {
>  		size = end_byte_offset + 1;
> -		oa_tc6_prcs_rx_frame_end(tc6, data, size);
> -		return 0;
> +		return oa_tc6_prcs_rx_frame_end(tc6, data, size);
>  	}
>  
>  	/* Process the chunk with previous rx frame end and next rx frame

[High]
Now that oa_tc6_prcs_rx_frame_end() returns int (and can return -EAGAIN
via oa_tc6_update_rx_skb() when tail + size > end), should the
"previous rx frame end + next rx frame start" branch also observe the
return value?  That branch is not shown in the diff, but in the
resulting file it reads:

    if (start_valid && end_valid && start_byte_offset > end_byte_offset) {
        if (tc6->rx_skb) {
            size = end_byte_offset + 1;
            oa_tc6_prcs_rx_frame_end(tc6, data, size);
        }
        size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
        return oa_tc6_prcs_rx_frame_start(tc6,
                                          &data[start_byte_offset],
                                          size);
    }

If oa_tc6_prcs_rx_frame_end() here returns -EAGAIN, oa_tc6_look_for_new_frame()
has already set tc6->rx_buf_overflow = true and cleared rx_skb.  Execution
then falls through to oa_tc6_prcs_rx_frame_start(), which allocates a
fresh rx_skb and copies the start bytes, but tc6->rx_buf_overflow stays
true because the guard

    if (start_valid && tc6->rx_buf_overflow)
        tc6->rx_buf_overflow = false;

is only evaluated on chunk entry.  Subsequent ongoing chunks (no
start_valid) then hit the earlier

    if (tc6->rx_buf_overflow)
        return 0;

path and are silently dropped, and the partial rx_skb is later thrown
away by the next allocate_rx_skb() -> cleanup_ongoing_rx_skb(), which
also increments rx_dropped.  Is this the intended behaviour for the
oversubscribed/chunk-loss scenario the patch is targeting?

[ ... ]

> @@ -972,8 +1004,9 @@ static u32 oa_tc6_get_rx_chunk_footer(struct oa_tc6 *tc6, u16 footer_offset)
>  static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  {
>  	u16 no_of_rx_chunks = length / OA_TC6_CHUNK_SIZE;
> +	bool retry = false;
> +	int ret = 0;
>  	u32 footer;
> -	int ret;
>  
>  	/* All the rx chunks in the receive SPI data buffer are examined here */
>  	for (int i = 0; i < no_of_rx_chunks; i++) {
> @@ -982,8 +1015,11 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  						    OA_TC6_CHUNK_PAYLOAD_SIZE);
>  
>  		ret = oa_tc6_process_rx_chunk_footer(tc6, footer);
> -		if (ret)
> -			return ret;
> +		if (ret) {
> +			if (ret != -EAGAIN)
> +				return ret;
> +			retry = true;
> +		}
>  
>  		/* If there is a data valid chunks then process it for the
>  		 * information needed to determine the validity and the location
> @@ -995,12 +1031,25 @@ static int oa_tc6_process_spi_data_rx_buf(struct oa_tc6 *tc6, u16 length)
>  
>  			ret = oa_tc6_prcs_rx_chunk_payload(tc6, payload,
>  							   footer);
> -			if (ret)
> -				return ret;
> +			if (ret) {
> +				if (ret != -ENOMEM && ret != -EAGAIN)
> +					return ret;
> +				retry = true;
> +			}
>  		}
>  	}

[Medium]
Is it intentional that a real -ENOMEM from oa_tc6_prcs_rx_chunk_payload()
(originating in oa_tc6_allocate_rx_skb() -> netdev_alloc_skb_ip_align())
is now folded into the retry path and converted to -EAGAIN at the end
of this function?

Before this patch, -ENOMEM propagated up through oa_tc6_try_spi_transfer()
and produced a netdev_err("Device error: %d", ret) plus
oa_tc6_free_ongoing_skbs() cleanup.  With this change, the outer while
loop treats -EAGAIN as non-fatal, so sustained memory exhaustion is
reported only as increments to stats.rx_dropped from
oa_tc6_allocate_rx_skb().

The commit message describes the change as being about buffer overflow
errors; is the change in visibility for allocation failures intended?

>  
> -	return 0;
> +	/* As a recoverable error, not bailing out on error code
> +	 * -EAGAIN. If subsequent loop iterations, if any, succeeds,
> +	 * error code would be overwritten. retry flag helps to
> +	 * make the caller to continue and retry.
> +	 */
> +	if (retry) {
> +		ret = -EAGAIN;
> +		oa_tc6_look_for_new_frame(tc6);
> +	}
> +
> +	return ret;
>  }

[High]
Can this unconditional oa_tc6_look_for_new_frame() discard a frame that
was successfully started later in the same buffer?

Consider a buffer where iteration N returns -EAGAIN from
oa_tc6_process_rx_chunk_footer() due to STATUS0 rx overflow (which
already sets rx_buf_overflow and clears rx_skb), and a later iteration
N+K carries a start_valid chunk that clears rx_buf_overflow via

    if (start_valid && tc6->rx_buf_overflow)
        tc6->rx_buf_overflow = false;

allocates a fresh rx_skb in oa_tc6_prcs_rx_frame_start(), and writes
the start bytes without further error.  retry is still true from
iteration N, so on loop exit the terminal oa_tc6_look_for_new_frame()
frees that newly-started rx_skb (incrementing rx_dropped) and re-sets
rx_buf_overflow = true.

On the next SPI transfer, the ongoing chunks of that frame are then
dropped by the top-of-function

    if (tc6->rx_buf_overflow)
        return 0;

guard in oa_tc6_prcs_rx_chunk_payload() until yet another start_valid
arrives.  Would it be safer to only call oa_tc6_look_for_new_frame()
here if the last iteration was still in an errored state, e.g. by
tracking the outcome of the most recent chunk rather than an aggregate
retry flag?

[ ... ]