| Message ID | 20241016105310.3500279-1-yuehaibing@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog() | expand |
On Wed, Oct 16, 2024 at 06:53:10PM +0800, Yue Haibing wrote: > Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS, > which is zero, this fix smatch warnings: > drivers/net/ethernet/intel/igc/igc_main.c:2533 > igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' > > Fixes: 26575105d6ed ("igc: Add initial XDP support") > Signed-off-by: Yue Haibing <yuehaibing@huawei.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 6e70bca15db1..c3d6e20c0be0 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter, > res = __igc_xdp_run_prog(adapter, prog, xdp); > > out: > - return ERR_PTR(-res); > + return res ? ERR_PTR(-res) : NULL; I think this is what PTR_ERR_OR_ZERO() is for.
On 10/16/2024 11:53 AM, Simon Horman wrote: > On Wed, Oct 16, 2024 at 06:53:10PM +0800, Yue Haibing wrote: >> Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS, >> which is zero, this fix smatch warnings: >> drivers/net/ethernet/intel/igc/igc_main.c:2533 >> igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' >> >> Fixes: 26575105d6ed ("igc: Add initial XDP support") >> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> >> --- >> drivers/net/ethernet/intel/igc/igc_main.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 6e70bca15db1..c3d6e20c0be0 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter, >> res = __igc_xdp_run_prog(adapter, prog, xdp); >> >> out: >> - return ERR_PTR(-res); >> + return res ? ERR_PTR(-res) : NULL; > > I think this is what PTR_ERR_OR_ZERO() is for. Not quite. PTR_ERR_OR_ZERO is intended for the case where you are extracting an error from a pointer. This is converting an error into a pointer. I am not sure what is really expected here. If res is zero, shouldn't we be returning an skb pointer and not NULL? Why does igc_xdp_run_prog even return a sk_buff pointer at all? It never actually returns an skb... This feels like the wrong fix entirely. __igc_xdp_run_prog returns a custom value for the action, between IGC_XDP_PASS, IGC_XDP_TX, IGC_XDP_REDIRECT, or IGC_XDP_CONSUMED. This function is called by igc_xdp_run_prog which converts this to a negative error code with the sk_buff pointer type. All so that we can assign a value to the skb pointer in ice_clean_rx_irq, and check it with IS_ERR I don't like this fix, I think we could drop the igc_xdp_run_prog wrapper, call __igc_xdp_run_prog directly and check its return value instead of this method of using an error pointer.
On 10/16/2024 4:06 PM, Jacob Keller wrote: > > > On 10/16/2024 11:53 AM, Simon Horman wrote: >> On Wed, Oct 16, 2024 at 06:53:10PM +0800, Yue Haibing wrote: >>> Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS, >>> which is zero, this fix smatch warnings: >>> drivers/net/ethernet/intel/igc/igc_main.c:2533 >>> igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' >>> >>> Fixes: 26575105d6ed ("igc: Add initial XDP support") >>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> >>> --- >>> drivers/net/ethernet/intel/igc/igc_main.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >>> index 6e70bca15db1..c3d6e20c0be0 100644 >>> --- a/drivers/net/ethernet/intel/igc/igc_main.c >>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >>> @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter, >>> res = __igc_xdp_run_prog(adapter, prog, xdp); >>> >>> out: >>> - return ERR_PTR(-res); >>> + return res ? ERR_PTR(-res) : NULL; >> >> I think this is what PTR_ERR_OR_ZERO() is for. > > Not quite. PTR_ERR_OR_ZERO is intended for the case where you are > extracting an error from a pointer. This is converting an error into a > pointer. > > I am not sure what is really expected here. If res is zero, shouldn't we > be returning an skb pointer and not NULL? > > Why does igc_xdp_run_prog even return a sk_buff pointer at all? It never > actually returns an skb... > > This feels like the wrong fix entirely. > > __igc_xdp_run_prog returns a custom value for the action, between > IGC_XDP_PASS, IGC_XDP_TX, IGC_XDP_REDIRECT, or IGC_XDP_CONSUMED. > > This function is called by igc_xdp_run_prog which converts this to a > negative error code with the sk_buff pointer type. > > All so that we can assign a value to the skb pointer in > ice_clean_rx_irq, and check it with IS_ERR > > I don't like this fix, I think we could drop the igc_xdp_run_prog > wrapper, call __igc_xdp_run_prog directly and check its return value > instead of this method of using an error pointer. Indeed, this SKB error stuff was added by 26575105d6ed ("igc: Add initial XDP support") which claims to be aligning with other Intel drivers. But the other Intel drivers just have a function that returns the xdp result and checks it directly. Perhaps this is due to the way that the igc driver shares rings between XDP and the regular path? Its not clear to me, but I think this fix is not what I would do.
On 2024/10/17 7:12, Jacob Keller wrote: > > > On 10/16/2024 4:06 PM, Jacob Keller wrote: >> >> >> On 10/16/2024 11:53 AM, Simon Horman wrote: >>> On Wed, Oct 16, 2024 at 06:53:10PM +0800, Yue Haibing wrote: >>>> Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS, >>>> which is zero, this fix smatch warnings: >>>> drivers/net/ethernet/intel/igc/igc_main.c:2533 >>>> igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' >>>> >>>> Fixes: 26575105d6ed ("igc: Add initial XDP support") >>>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> >>>> --- >>>> drivers/net/ethernet/intel/igc/igc_main.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >>>> index 6e70bca15db1..c3d6e20c0be0 100644 >>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c >>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >>>> @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter, >>>> res = __igc_xdp_run_prog(adapter, prog, xdp); >>>> >>>> out: >>>> - return ERR_PTR(-res); >>>> + return res ? ERR_PTR(-res) : NULL; >>> >>> I think this is what PTR_ERR_OR_ZERO() is for. >> >> Not quite. PTR_ERR_OR_ZERO is intended for the case where you are >> extracting an error from a pointer. This is converting an error into a >> pointer. >> >> I am not sure what is really expected here. If res is zero, shouldn't we >> be returning an skb pointer and not NULL? >> >> Why does igc_xdp_run_prog even return a sk_buff pointer at all? It never >> actually returns an skb... >> >> This feels like the wrong fix entirely. >> >> __igc_xdp_run_prog returns a custom value for the action, between >> IGC_XDP_PASS, IGC_XDP_TX, IGC_XDP_REDIRECT, or IGC_XDP_CONSUMED. >> >> This function is called by igc_xdp_run_prog which converts this to a >> negative error code with the sk_buff pointer type. >> >> All so that we can assign a value to the skb pointer in >> ice_clean_rx_irq, and check it with IS_ERR >> >> I don't like this fix, I think we could drop the igc_xdp_run_prog >> wrapper, call __igc_xdp_run_prog directly and check its return value >> instead of this method of using an error pointer. > > Indeed, this SKB error stuff was added by 26575105d6ed ("igc: Add > initial XDP support") which claims to be aligning with other Intel drivers. > > But the other Intel drivers just have a function that returns the xdp > result and checks it directly. >Thanks for review，maybe can fix this as commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")? > Perhaps this is due to the way that the igc driver shares rings between > XDP and the regular path? > > Its not clear to me, but I think this fix is not what I would do. > > .
On 2024/10/17 7:12, Jacob Keller wrote: > > > On 10/16/2024 4:06 PM, Jacob Keller wrote: >> >> >> On 10/16/2024 11:53 AM, Simon Horman wrote: >>> On Wed, Oct 16, 2024 at 06:53:10PM +0800, Yue Haibing wrote: >>>> Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS, >>>> which is zero, this fix smatch warnings: >>>> drivers/net/ethernet/intel/igc/igc_main.c:2533 >>>> igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' >>>> >>>> Fixes: 26575105d6ed ("igc: Add initial XDP support") >>>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> >>>> --- >>>> drivers/net/ethernet/intel/igc/igc_main.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >>>> index 6e70bca15db1..c3d6e20c0be0 100644 >>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c >>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >>>> @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter, >>>> res = __igc_xdp_run_prog(adapter, prog, xdp); >>>> >>>> out: >>>> - return ERR_PTR(-res); >>>> + return res ? ERR_PTR(-res) : NULL; >>> >>> I think this is what PTR_ERR_OR_ZERO() is for. >> >> Not quite. PTR_ERR_OR_ZERO is intended for the case where you are >> extracting an error from a pointer. This is converting an error into a >> pointer. >> >> I am not sure what is really expected here. If res is zero, shouldn't we >> be returning an skb pointer and not NULL? >> >> Why does igc_xdp_run_prog even return a sk_buff pointer at all? It never >> actually returns an skb... >> >> This feels like the wrong fix entirely. >> >> __igc_xdp_run_prog returns a custom value for the action, between >> IGC_XDP_PASS, IGC_XDP_TX, IGC_XDP_REDIRECT, or IGC_XDP_CONSUMED. >> >> This function is called by igc_xdp_run_prog which converts this to a >> negative error code with the sk_buff pointer type. >> >> All so that we can assign a value to the skb pointer in >> ice_clean_rx_irq, and check it with IS_ERR >> >> I don't like this fix, I think we could drop the igc_xdp_run_prog >> wrapper, call __igc_xdp_run_prog directly and check its return value >> instead of this method of using an error pointer. > > Indeed, this SKB error stuff was added by 26575105d6ed ("igc: Add > initial XDP support") which claims to be aligning with other Intel drivers. > Thanks for review，maybe can fix this as commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")? > But the other Intel drivers just have a function that returns the xdp > result and checks it directly. > > Perhaps this is due to the way that the igc driver shares rings between > XDP and the regular path? > > Its not clear to me, but I think this fix is not what I would do. > > .
On Thu, Oct 17, 2024 at 11:55:05AM +0800, Yue Haibing wrote: > On 2024/10/17 7:12, Jacob Keller wrote: > > > > > > On 10/16/2024 4:06 PM, Jacob Keller wrote: > >> > >> > >> On 10/16/2024 11:53 AM, Simon Horman wrote: > >>> On Wed, Oct 16, 2024 at 06:53:10PM +0800, Yue Haibing wrote: > >>>> Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS, > >>>> which is zero, this fix smatch warnings: > >>>> drivers/net/ethernet/intel/igc/igc_main.c:2533 > >>>> igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' > >>>> > >>>> Fixes: 26575105d6ed ("igc: Add initial XDP support") > >>>> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> > >>>> --- > >>>> drivers/net/ethernet/intel/igc/igc_main.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > >>>> index 6e70bca15db1..c3d6e20c0be0 100644 > >>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c > >>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c > >>>> @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter, > >>>> res = __igc_xdp_run_prog(adapter, prog, xdp); > >>>> > >>>> out: > >>>> - return ERR_PTR(-res); > >>>> + return res ? ERR_PTR(-res) : NULL; > >>> > >>> I think this is what PTR_ERR_OR_ZERO() is for. > >> > >> Not quite. PTR_ERR_OR_ZERO is intended for the case where you are > >> extracting an error from a pointer. This is converting an error into a > >> pointer. > >> > >> I am not sure what is really expected here. If res is zero, shouldn't we > >> be returning an skb pointer and not NULL? > >> > >> Why does igc_xdp_run_prog even return a sk_buff pointer at all? It never > >> actually returns an skb... > >> > >> This feels like the wrong fix entirely. > >> > >> __igc_xdp_run_prog returns a custom value for the action, between > >> IGC_XDP_PASS, IGC_XDP_TX, IGC_XDP_REDIRECT, or IGC_XDP_CONSUMED. > >> > >> This function is called by igc_xdp_run_prog which converts this to a > >> negative error code with the sk_buff pointer type. > >> > >> All so that we can assign a value to the skb pointer in > >> ice_clean_rx_irq, and check it with IS_ERR > >> > >> I don't like this fix, I think we could drop the igc_xdp_run_prog > >> wrapper, call __igc_xdp_run_prog directly and check its return value > >> instead of this method of using an error pointer. > > > > Indeed, this SKB error stuff was added by 26575105d6ed ("igc: Add > > initial XDP support") which claims to be aligning with other Intel drivers. > > > > Thanks for review，maybe can fix this as commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")? Yes please get rid of this logic. Historically speaking, i40e started this and other drivers followed, but I chose in ice implementation to avoid that :) Kurt, if you'll be sending next revision for igb xsk support, then avoid the logic we talk about here as well, please. > > > But the other Intel drivers just have a function that returns the xdp > > result and checks it directly. > > > > Perhaps this is due to the way that the igc driver shares rings between > > XDP and the regular path? > > > > Its not clear to me, but I think this fix is not what I would do. > > > > .
On Wed, Oct 16, 2024 at 04:06:34PM -0700, Jacob Keller wrote: > > > On 10/16/2024 11:53 AM, Simon Horman wrote: > > On Wed, Oct 16, 2024 at 06:53:10PM +0800, Yue Haibing wrote: > >> Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS, > >> which is zero, this fix smatch warnings: > >> drivers/net/ethernet/intel/igc/igc_main.c:2533 > >> igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' > >> > >> Fixes: 26575105d6ed ("igc: Add initial XDP support") > >> Signed-off-by: Yue Haibing <yuehaibing@huawei.com> > >> --- > >> drivers/net/ethernet/intel/igc/igc_main.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > >> index 6e70bca15db1..c3d6e20c0be0 100644 > >> --- a/drivers/net/ethernet/intel/igc/igc_main.c > >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c > >> @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter, > >> res = __igc_xdp_run_prog(adapter, prog, xdp); > >> > >> out: > >> - return ERR_PTR(-res); > >> + return res ? ERR_PTR(-res) : NULL; > > > > I think this is what PTR_ERR_OR_ZERO() is for. > > Not quite. PTR_ERR_OR_ZERO is intended for the case where you are > extracting an error from a pointer. This is converting an error into a > pointer. Yes, silly me. > I am not sure what is really expected here. If res is zero, shouldn't we > be returning an skb pointer and not NULL? Right. I think the whole point of the cited warning is that it highlights code that is often buggy. I think I may have tried to address it in the past, but if so unsuccessfully. In any case, I do think it would be good to dig into this and either fix it properly (or understand why it is correct and note that somewhere. > > Why does igc_xdp_run_prog even return a sk_buff pointer at all? It never > actually returns an skb... > > This feels like the wrong fix entirely. > > __igc_xdp_run_prog returns a custom value for the action, between > IGC_XDP_PASS, IGC_XDP_TX, IGC_XDP_REDIRECT, or IGC_XDP_CONSUMED. > > This function is called by igc_xdp_run_prog which converts this to a > negative error code with the sk_buff pointer type. > > All so that we can assign a value to the skb pointer in > ice_clean_rx_irq, and check it with IS_ERR > > I don't like this fix, I think we could drop the igc_xdp_run_prog > wrapper, call __igc_xdp_run_prog directly and check its return value > instead of this method of using an error pointer.
On 10/17/2024 7:16 AM, Simon Horman wrote: > On Wed, Oct 16, 2024 at 04:06:34PM -0700, Jacob Keller wrote: >> >> Not quite. PTR_ERR_OR_ZERO is intended for the case where you are >> extracting an error from a pointer. This is converting an error into a >> pointer. > > Yes, silly me. > >> I am not sure what is really expected here. If res is zero, shouldn't we >> be returning an skb pointer and not NULL? > > Right. I think the whole point of the cited warning is that it highlights > code that is often buggy. I think I may have tried to address it in the > past, but if so unsuccessfully. In any case, I do think it would be good to > dig into this and either fix it properly (or understand why it is correct > and note that somewhere. > Right. I think we identified the correct fix. This same code was in i40e and was removed in a better way.
On 10/17/2024 4:03 AM, Maciej Fijalkowski wrote: > On Thu, Oct 17, 2024 at 11:55:05AM +0800, Yue Haibing wrote: >> On 2024/10/17 7:12, Jacob Keller wrote: >>> On 10/16/2024 4:06 PM, Jacob Keller wrote: >>>> I don't like this fix, I think we could drop the igc_xdp_run_prog >>>> wrapper, call __igc_xdp_run_prog directly and check its return value >>>> instead of this method of using an error pointer. >>> >>> Indeed, this SKB error stuff was added by 26575105d6ed ("igc: Add >>> initial XDP support") which claims to be aligning with other Intel drivers. >>> >> >> Thanks for review，maybe can fix this as commit 12738ac4754e ("i40e: Fix sparse errors in i40e_txrx.c")? > > Yes please get rid of this logic. Historically speaking, i40e started this > and other drivers followed, but I chose in ice implementation to avoid > that :) Thanks! > > Kurt, if you'll be sending next revision for igb xsk support, then avoid > the logic we talk about here as well, please. > Yes, please fix this the way i40e did in the mentioned commit above. That looks significantly better to me :)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 6e70bca15db1..c3d6e20c0be0 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -2530,7 +2530,7 @@ static struct sk_buff *igc_xdp_run_prog(struct igc_adapter *adapter, res = __igc_xdp_run_prog(adapter, prog, xdp); out: - return ERR_PTR(-res); + return res ? ERR_PTR(-res) : NULL; } /* This function assumes __netif_tx_lock is held by the caller. */
Return NULL instead of passing to ERR_PTR while res is IGC_XDP_PASS, which is zero, this fix smatch warnings: drivers/net/ethernet/intel/igc/igc_main.c:2533 igc_xdp_run_prog() warn: passing zero to 'ERR_PTR' Fixes: 26575105d6ed ("igc: Add initial XDP support") Signed-off-by: Yue Haibing <yuehaibing@huawei.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)