Re: [PATCH net] net: ethernet: adi: adin1110: Fix some error handling path in adin1110_read_fifo()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Le 04/10/2024 à 13:37, Simon Horman a écrit :
On Thu, Oct 03, 2024 at 08:53:15PM +0200, Christophe JAILLET wrote:
If 'frame_size' is too small or if 'round_len' is an error code, it is
likely that an error code should be returned to the caller.

Actually, 'ret' is likely to be 0, so if one of these sanity checks fails,
'success' is returned.

Hi Christophe,

I think we can say "'ret' will be 0".

Agreed.

	ret = adin1110_read_reg()
	--> spi_sync_transfer()
	--> spi_sync()

which explicitly documents "zero on success, else a negative error code."

At least that is what my brief investigation tells me.


Return -EINVAL instead.


If the patch is considered as correct, can you confirm that -EINVAL is the correct error code to use? If not, which one would be preferred?


Please include some information on how this was found and tested.
e.g.

Found by inspection / Found using widget-ng.

I would say: found by luck! :)

The explanation below will be of no help in the commit message and won't be added. I just give you all the gory details because you asked for it ;-)

(and after reading bellow, you can call me crazy!)



I was looking at functions that propagate error codes as their last argument. The idea came after submitting [1].

I read cci_read() and wondered if functions with such a semantic could use an un-initialized last argument. In such a case, this function could not behave as expected if the initial value of "err" was not 0.

So I wrote the following coccinelle script and several other variations.


// Options: --include-headers

@ok@
identifier fct, err;
type T;
@@

	int fct(..., T *err)
	{
		...
	}

@test depends on ok@
identifier x, fct = ok.fct;
expression res;
type T = ok.T;
@@

*	T x;
	...
(
	fct(..., &x);
|
	res = fct(..., &x);
)

(For the record, I have not found any issue with it...)


BUT, adin1110_read_fifo() was spotted because of the prototype of adin1110_read_reg().

When I reviewed the code, I quickly saw that it was a false positive and that using "type T" in my script was not that logical...

Anyway, when reviewing the code, I saw:

	if (ret < 0)
		return ret;

	/* The read frame size includes the extra 2 bytes
	 * from the  ADIN1110 frame header.
	 */
	if (frame_size < ADIN1110_FRAME_HEADER_LEN + ADIN1110_FEC_LEN)
		return ret;

	round_len = adin1110_round_len(frame_size);
	if (round_len < 0)
		return ret;

which looks really strange and likely broken...

Then I sent the patch we are talking about!


(yes some real people really search such things and write such coccinelle scripts, and now you can call me crazy)


[1]: https://lore.kernel.org/all/666ac169157f0af1c2e1d47926b68870cb39d587.1727977974.git.christophe.jaillet@xxxxxxxxxx/

Compile tested only.

As a "speculative" patch, it was only compile tested, you are correct.



Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
---
This patch is speculative.
If returning 0 is what was intended, then an explicit 0 would be better.

In my brief investigation I see that adin1110_read_fifo()
is only called by adin1110_read_frames(), like this:

	while (budget) {
		...

		ret = adin1110_read_fifo(port_priv);
		if (ret < 0)
			return;

		budget--;
	}

So the question becomes, should a failure in reading the fifo,
because of an invalid frame size, be treated as an error
and terminate reading frames.

Like you, I speculate the answer is yes.
But I think we need a bit more certainty to take this patch.

I won't be of any help here.

I can just say that "it looks strange" and is "certainly" bogus, but won't be able the prove it nor test it.


I'll wait a bit before sending a v2. If confirming this point is a requirement for accepting the patch, there is no need to urge for a v2 if no-one cares about answering your point.

CJ








[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux