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

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

[Christophe JAILLET]

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.

Return -EINVAL instead.

Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is speculative.
If returning 0 is what was intended, then an explicit 0 would be better.
---
 drivers/net/ethernet/adi/adin1110.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
index 3431a7e62b0d..c04036b687dd 100644
--- a/drivers/net/ethernet/adi/adin1110.c
+++ b/drivers/net/ethernet/adi/adin1110.c
@@ -318,11 +318,11 @@ static int adin1110_read_fifo(struct adin1110_port_priv *port_priv)
 	 * from the  ADIN1110 frame header.
 	 */
 	if (frame_size < ADIN1110_FRAME_HEADER_LEN + ADIN1110_FEC_LEN)
-		return ret;
+		return -EINVAL;
 
 	round_len = adin1110_round_len(frame_size);
 	if (round_len < 0)
-		return ret;
+		return -EINVAL;
 
 	frame_size_no_fcs = frame_size - ADIN1110_FRAME_HEADER_LEN - ADIN1110_FEC_LEN;
 	memset(priv->data, 0, ADIN1110_RD_HEADER_LEN);
-- 
2.46.2

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

[Simon Horman]

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".
At least that is what my brief investigation tells me.

> 
> Return -EINVAL instead.

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

Found by inspection / Found using widget-ng.
Compile tested only.

> 
> Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.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.

-- 
pw-bot: under-review

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

[Christophe JAILLET]

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@wanadoo.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


> 

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

[Simon Horman]

On Fri, Oct 04, 2024 at 03:15:39PM +0200, Christophe JAILLET wrote:
> 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?

-EINVAL seems reasonable to me.

> > 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!)

:)

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

[Dan Carpenter]

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.
> 
> Return -EINVAL instead.
> 
> Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is speculative.
> If returning 0 is what was intended, then an explicit 0 would be better.

I have an unpublished Smatch warning for these:

drivers/net/ethernet/adi/adin1110.c:321 adin1110_read_fifo() info: returning a literal zero is cleaner
drivers/net/ethernet/adi/adin1110.c:325 adin1110_read_fifo() info: returning a literal zero is cleaner

It's a pity that deliberately doing a "return ret;" when ret is zero is so
common.  Someone explained to me that it was "done deliberately to express that
we were propagating the success from frob_whatever()".  No no no!

I don't review these warnings unless I'm fixing a bug in the driver because
they're too common.  The only ones I review are:

	ret = frob();
	if (!ret)
		return ret;

Maybe 20% of the time those warnings indicate a reversed if statement.

Your heuristic here is very clever and I'll try steal it to create a new more
specific warning.

regards,
dan carpenter

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

[Christophe JAILLET]

Le 04/10/2024 à 13:47, Dan Carpenter 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.
>>
>> Return -EINVAL instead.
>>
>> Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> This patch is speculative.
>> If returning 0 is what was intended, then an explicit 0 would be better.
> 
> I have an unpublished Smatch warning for these:
> 
> drivers/net/ethernet/adi/adin1110.c:321 adin1110_read_fifo() info: returning a literal zero is cleaner
> drivers/net/ethernet/adi/adin1110.c:325 adin1110_read_fifo() info: returning a literal zero is cleaner
> 
> It's a pity that deliberately doing a "return ret;" when ret is zero is so
> common.  Someone explained to me that it was "done deliberately to express that
> we were propagating the success from frob_whatever()".  No no no!
> 
> I don't review these warnings unless I'm fixing a bug in the driver because
> they're too common.  The only ones I review are:
> 
> 	ret = frob();
> 	if (!ret)
> 		return ret;
> 
> Maybe 20% of the time those warnings indicate a reversed if statement.
> 
> Your heuristic here is very clever and I'll try steal it to create a new more
> specific warning.

Well my heuristic is mostly luck, here ;-)

Anyway, what I was looking for (un-initialized last argument in some 
function) could be nice to have in smatch, if not already present.


Finally, if you want, I can give a look at the warnings if you share the 
log.

CJ

> 
> regards,
> dan carpenter
> 
> 
> 

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

[Jakub Kicinski]

On Fri, 4 Oct 2024 14:47:22 +0300 Dan Carpenter wrote:
> It's a pity that deliberately doing a "return ret;" when ret is zero is so
> common.  Someone explained to me that it was "done deliberately to express that
> we were propagating the success from frob_whatever()".  No no no!

FWIW I pitched to Linus that we should have a err_t of some sort for
int variables which must never be returned with value of 0.
He wasn't impressed, but I still think it would be useful :)

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

[Simon Horman]

On Fri, Oct 04, 2024 at 11:09:52AM -0700, Jakub Kicinski wrote:
> On Fri, 4 Oct 2024 14:47:22 +0300 Dan Carpenter wrote:
> > It's a pity that deliberately doing a "return ret;" when ret is zero is so
> > common.  Someone explained to me that it was "done deliberately to express that
> > we were propagating the success from frob_whatever()".  No no no!
> 
> FWIW I pitched to Linus that we should have a err_t of some sort for
> int variables which must never be returned with value of 0.
> He wasn't impressed, but I still think it would be useful :)

FWIIW, I think something like that would be quite nice.

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

[Julia Lawall]

On Mon, 7 Oct 2024, Simon Horman wrote:

> On Fri, Oct 04, 2024 at 11:09:52AM -0700, Jakub Kicinski wrote:
> > On Fri, 4 Oct 2024 14:47:22 +0300 Dan Carpenter wrote:
> > > It's a pity that deliberately doing a "return ret;" when ret is zero is so
> > > common.  Someone explained to me that it was "done deliberately to express that
> > > we were propagating the success from frob_whatever()".  No no no!
> >
> > FWIW I pitched to Linus that we should have a err_t of some sort for
> > int variables which must never be returned with value of 0.
> > He wasn't impressed, but I still think it would be useful :)
>
> FWIIW, I think something like that would be quite nice.

Likewise.

julia

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  3 Oct 2024 20:53:15 +0200 you 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.
> 
> Return -EINVAL instead.
> 
> [...]

Here is the summary with links:
  - [net] net: ethernet: adi: adin1110: Fix some error handling path in adin1110_read_fifo()
    https://git.kernel.org/netdev/net/c/83211ae16405

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html