| Message ID | 8ff73b40f50d8fa994a454911b66adebce8da266.1727981562.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 83211ae1640516accae645de82f5a0a142676897 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ethernet: adi: adin1110: Fix some error handling path in adin1110_read_fifo() | expand |
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.
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
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 >
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 > > >
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 :)
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!) :)
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.
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
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!
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);
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(-)