From: Colin King <hidden> Date: 2017-06-02 14:58:31
From: Colin Ian King <redacted>
The current comparison of entry < 0 will never be true since entry is an
unsigned integer. Cast entry to an int to ensure -ve error return values
from the call to jumbo_frm are correctly being caught.
Detected by CoverityScan, CID#1238760 ("Macro compares unsigned to 0")
Signed-off-by: Colin Ian King <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Andy Shevchenko <hidden> Date: 2017-06-03 15:56:00
On Fri, Jun 2, 2017 at 5:58 PM, Colin King [off-list ref] wrote:
The current comparison of entry < 0 will never be true since entry is an
unsigned integer. Cast entry to an int to ensure -ve error return values
from the call to jumbo_frm are correctly being caught.
quoted hunk
if (unlikely(is_jumbo) && likely(priv->synopsys_id < DWMAC_CORE_4_00)) { entry = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);- if (unlikely(entry < 0))+ if (unlikely((int)entry < 0))
It feels like a hiding some other issue.
--
With Best Regards,
Andy Shevchenko
From: Colin Ian King <hidden> Date: 2017-06-03 16:35:26
On 03/06/17 16:55, Andy Shevchenko wrote:
On Fri, Jun 2, 2017 at 5:58 PM, Colin King [off-list ref] wrote:
quoted
The current comparison of entry < 0 will never be true since entry is an
unsigned integer. Cast entry to an int to ensure -ve error return values
from the call to jumbo_frm are correctly being caught.
quoted
if (unlikely(is_jumbo) && likely(priv->synopsys_id < DWMAC_CORE_4_00)) { entry = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);- if (unlikely(entry < 0))+ if (unlikely((int)entry < 0))
It feels like a hiding some other issue.
The alternative is:
int rc = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);
if (unlikely(rc < 0))
goto dma_map_err;
entry = rc;
however, that is effectively the same. The cast I'm using is a well used
idiom in the kernel, it used in almost a hundred similar cases.
git grep "< 0" | grep "(int)" | wc -l
95
Colin
From: Julia Lawall <hidden> Date: 2017-06-03 16:53:41
On Sat, 3 Jun 2017, Colin Ian King wrote:
On 03/06/17 16:55, Andy Shevchenko wrote:
quoted
On Fri, Jun 2, 2017 at 5:58 PM, Colin King [off-list ref] wrote:
quoted
The current comparison of entry < 0 will never be true since entry is an
unsigned integer. Cast entry to an int to ensure -ve error return values
from the call to jumbo_frm are correctly being caught.
quoted
if (unlikely(is_jumbo) && likely(priv->synopsys_id < DWMAC_CORE_4_00)) { entry = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);- if (unlikely(entry < 0))+ if (unlikely((int)entry < 0))
It feels like a hiding some other issue.
The alternative is:
int rc = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);
if (unlikely(rc < 0))
goto dma_map_err;
entry = rc;
however, that is effectively the same. The cast I'm using is a well used
idiom in the kernel, it used in almost a hundred similar cases.
git grep "< 0" | grep "(int)" | wc -l
95
Does entry really have to be unsigned? The jumbo_frm function returns an
int, not an unsigned int, so it seems unpleasant to make it unsigned
prematurely just to put a cast afterwards. The remaining computation
seems to involve only small numbers.
julia
From: Andy Shevchenko <hidden> Date: 2017-06-03 17:18:36
On Sat, Jun 3, 2017 at 7:35 PM, Colin Ian King [off-list ref] wrote:
On 03/06/17 16:55, Andy Shevchenko wrote:
quoted
On Fri, Jun 2, 2017 at 5:58 PM, Colin King [off-list ref] wrote:
quoted
The current comparison of entry < 0 will never be true since entry is an
unsigned integer. Cast entry to an int to ensure -ve error return values
from the call to jumbo_frm are correctly being caught.
quoted
if (unlikely(is_jumbo) && likely(priv->synopsys_id < DWMAC_CORE_4_00)) { entry = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);- if (unlikely(entry < 0))+ if (unlikely((int)entry < 0))
It feels like a hiding some other issue.
The alternative is:
int rc = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion);
if (unlikely(rc < 0))
goto dma_map_err;
entry = rc;
Looks cleaner, though I was thinking about changing type of entry as
Julia suggested. Needs to be checked carefully anyway.
however, that is effectively the same. The cast I'm using is a well used
idiom in the kernel, it used in almost a hundred similar cases.
git grep "< 0" | grep "(int)" | wc -l
95
With refined grep I'e got only 56 so far. And I see it's spread only
in 45 modules, most cases are single use cases.
So, I hardly can tell this is an idiom in kernel.
--
With Best Regards,
Andy Shevchenko
From: David Miller <davem@davemloft.net> Date: 2017-06-04 23:58:25
From: Colin King <redacted>
Date: Fri, 2 Jun 2017 15:58:27 +0100
From: Colin Ian King <redacted>
The current comparison of entry < 0 will never be true since entry is an
unsigned integer. Cast entry to an int to ensure -ve error return values
from the call to jumbo_frm are correctly being caught.
Detected by CoverityScan, CID#1238760 ("Macro compares unsigned to 0")
Signed-off-by: Colin Ian King <redacted>
Like others have suggested, probably making 'entry' signed is
a better fix.
I was initially worried that STMMAC_GET_ENTRY() might become
more expensive if it was implemented using '%' but it is using
'and' masking instead which doesn't have that kind of problem.