[PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Yue Haibing]

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;
 }
 
 /* This function assumes __netif_tx_lock is held by the caller. */
-- 
2.34.1

Re: [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Simon Horman]

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.

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Jacob Keller]

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.

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Jacob Keller]

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.

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Yue Haibing]

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

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Yue Haibing]

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

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Maciej Fijalkowski]

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

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Jacob Keller]

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

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On Thu Oct 17 2024, Jacob Keller wrote:
> 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 :)

Changed the return type of igb_run_xdp_zc() from skb* to int.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Simon Horman]

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.

-- 
pw-bot: changes-requested

Re: [Intel-wired-lan] [PATCH net] igc: Fix passing 0 to ERR_PTR in igc_xdp_run_prog()

[Jacob Keller]

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.