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

[Yue Haibing <yuehaibing@huawei.com>]

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