[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2669052d-752f-416a-9d5e-a03848f30904@wanadoo.fr>
Date: Fri, 4 Oct 2024 15:15:39 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Simon Horman <horms@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Lennart Franzen <lennart@...omain.com>,
Alexandru Tachici <alexandru.tachici@...log.com>,
linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH net] net: ethernet: adi: adin1110: Fix some error handling
path in adin1110_read_fifo()
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@wanadoo.fr/
> 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@...adoo.fr>
>> ---
>> 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
>
Powered by blists - more mailing lists