* [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value @ 2017-06-02 14:58 ` Colin King 0 siblings, 0 replies; 12+ messages in thread From: Colin King @ 2017-06-02 14:58 UTC (permalink / raw) To: Giuseppe Cavallaro, Alexandre Torgue, netdev Cc: kernel-janitors, linux-kernel From: Colin Ian King <colin.king@canonical.com> 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 <colin.king@canonical.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 68a188e74c54..5cc19506ba85 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2996,7 +2996,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) 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)) goto dma_map_err; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value @ 2017-06-02 14:58 ` Colin King 0 siblings, 0 replies; 12+ messages in thread From: Colin King @ 2017-06-02 14:58 UTC (permalink / raw) To: Giuseppe Cavallaro, Alexandre Torgue, netdev Cc: kernel-janitors, linux-kernel From: Colin Ian King <colin.king@canonical.com> 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 <colin.king@canonical.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 68a188e74c54..5cc19506ba85 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2996,7 +2996,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) 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)) goto dma_map_err; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value 2017-06-02 14:58 ` Colin King @ 2017-06-03 15:55 ` Andy Shevchenko -1 siblings, 0 replies; 12+ messages in thread From: Andy Shevchenko @ 2017-06-03 15:55 UTC (permalink / raw) To: Colin King Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev, kernel-janitors, linux-kernel On Fri, Jun 2, 2017 at 5:58 PM, Colin King <colin.king@canonical.com> 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. > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value @ 2017-06-03 15:55 ` Andy Shevchenko 0 siblings, 0 replies; 12+ messages in thread From: Andy Shevchenko @ 2017-06-03 15:55 UTC (permalink / raw) To: Colin King Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev, kernel-janitors, linux-kernel On Fri, Jun 2, 2017 at 5:58 PM, Colin King <colin.king@canonical.com> 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. > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value 2017-06-03 15:55 ` Andy Shevchenko @ 2017-06-03 16:35 ` Colin Ian King -1 siblings, 0 replies; 12+ messages in thread From: Colin Ian King @ 2017-06-03 16:35 UTC (permalink / raw) To: Andy Shevchenko Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev, kernel-janitors, linux-kernel On 03/06/17 16:55, Andy Shevchenko wrote: > On Fri, Jun 2, 2017 at 5:58 PM, Colin King <colin.king@canonical.com> 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. > >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value @ 2017-06-03 16:35 ` Colin Ian King 0 siblings, 0 replies; 12+ messages in thread From: Colin Ian King @ 2017-06-03 16:35 UTC (permalink / raw) To: Andy Shevchenko Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev, kernel-janitors, linux-kernel On 03/06/17 16:55, Andy Shevchenko wrote: > On Fri, Jun 2, 2017 at 5:58 PM, Colin King <colin.king@canonical.com> 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. > >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value 2017-06-03 16:35 ` Colin Ian King @ 2017-06-03 16:53 ` Julia Lawall -1 siblings, 0 replies; 12+ messages in thread From: Julia Lawall @ 2017-06-03 16:53 UTC (permalink / raw) To: Colin Ian King Cc: Andy Shevchenko, Giuseppe Cavallaro, Alexandre Torgue, netdev, kernel-janitors, linux-kernel On Sat, 3 Jun 2017, Colin Ian King wrote: > On 03/06/17 16:55, Andy Shevchenko wrote: > > On Fri, Jun 2, 2017 at 5:58 PM, Colin King <colin.king@canonical.com> 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. > > > >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value @ 2017-06-03 16:53 ` Julia Lawall 0 siblings, 0 replies; 12+ messages in thread From: Julia Lawall @ 2017-06-03 16:53 UTC (permalink / raw) To: Colin Ian King Cc: Andy Shevchenko, Giuseppe Cavallaro, Alexandre Torgue, netdev, kernel-janitors, linux-kernel On Sat, 3 Jun 2017, Colin Ian King wrote: > On 03/06/17 16:55, Andy Shevchenko wrote: > > On Fri, Jun 2, 2017 at 5:58 PM, Colin King <colin.king@canonical.com> 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. > > > >> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value 2017-06-03 16:35 ` Colin Ian King @ 2017-06-03 17:18 ` Andy Shevchenko -1 siblings, 0 replies; 12+ messages in thread From: Andy Shevchenko @ 2017-06-03 17:18 UTC (permalink / raw) To: Colin Ian King Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev, kernel-janitors, linux-kernel On Sat, Jun 3, 2017 at 7:35 PM, Colin Ian King <colin.king@canonical.com> wrote: > On 03/06/17 16:55, Andy Shevchenko wrote: >> On Fri, Jun 2, 2017 at 5:58 PM, Colin King <colin.king@canonical.com> 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. >> >>> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value @ 2017-06-03 17:18 ` Andy Shevchenko 0 siblings, 0 replies; 12+ messages in thread From: Andy Shevchenko @ 2017-06-03 17:18 UTC (permalink / raw) To: Colin Ian King Cc: Giuseppe Cavallaro, Alexandre Torgue, netdev, kernel-janitors, linux-kernel On Sat, Jun 3, 2017 at 7:35 PM, Colin Ian King <colin.king@canonical.com> wrote: > On 03/06/17 16:55, Andy Shevchenko wrote: >> On Fri, Jun 2, 2017 at 5:58 PM, Colin King <colin.king@canonical.com> 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. >> >>> 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value 2017-06-02 14:58 ` Colin King @ 2017-06-04 23:58 ` David Miller -1 siblings, 0 replies; 12+ messages in thread From: David Miller @ 2017-06-04 23:58 UTC (permalink / raw) To: colin.king Cc: peppe.cavallaro, alexandre.torgue, netdev, kernel-janitors, linux-kernel From: Colin King <colin.king@canonical.com> Date: Fri, 2 Jun 2017 15:58:27 +0100 > From: Colin Ian King <colin.king@canonical.com> > > 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 <colin.king@canonical.com> 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value @ 2017-06-04 23:58 ` David Miller 0 siblings, 0 replies; 12+ messages in thread From: David Miller @ 2017-06-04 23:58 UTC (permalink / raw) To: colin.king Cc: peppe.cavallaro, alexandre.torgue, netdev, kernel-janitors, linux-kernel From: Colin King <colin.king@canonical.com> Date: Fri, 2 Jun 2017 15:58:27 +0100 > From: Colin Ian King <colin.king@canonical.com> > > 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 <colin.king@canonical.com> 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-06-04 23:58 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-02 14:58 [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value Colin King 2017-06-02 14:58 ` Colin King 2017-06-03 15:55 ` Andy Shevchenko 2017-06-03 15:55 ` Andy Shevchenko 2017-06-03 16:35 ` Colin Ian King 2017-06-03 16:35 ` Colin Ian King 2017-06-03 16:53 ` Julia Lawall 2017-06-03 16:53 ` Julia Lawall 2017-06-03 17:18 ` Andy Shevchenko 2017-06-03 17:18 ` Andy Shevchenko 2017-06-04 23:58 ` David Miller 2017-06-04 23:58 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.