Re: [Intel-wired-lan] [bug report] ixgbe: add VF IPsec management

["Nelson, Shannon" <shannon.nelson@amd.com>]

On 2/14/2024 5:58 AM, Przemek Kitszel wrote:
> 
> On 2/9/24 18:57, Nelson, Shannon wrote:
>> On 2/9/2024 4:59 AM, Dan Carpenter wrote:
>>>
>>> Hello Shannon Nelson,
>>>
>>> The patch eda0333ac293: "ixgbe: add VF IPsec management" from Aug 13,
>>> 2018 (linux-next), leads to the following Smatch static checker
>>> warning:
>>>
>>>          drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c:917
>>> ixgbe_ipsec_vf_add_sa()
>>>          warn: sleeping in IRQ context
>>>
>>> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c
>>>      890 int ixgbe_ipsec_vf_add_sa(struct ixgbe_adapter *adapter, u32
>>> *msgbuf, u32 vf)
>>>      891 {
>>>      892         struct ixgbe_ipsec *ipsec = adapter->ipsec;
>>>      893         struct xfrm_algo_desc *algo;
>>>      894         struct sa_mbx_msg *sam;
>>>      895         struct xfrm_state *xs;
>>>      896         size_t aead_len;
>>>      897         u16 sa_idx;
>>>      898         u32 pfsa;
>>>      899         int err;
>>>      900
>>>      901         sam = (struct sa_mbx_msg *)(&msgbuf[1]);
>>>      902         if (!adapter->vfinfo[vf].trusted ||
>>>      903             !(adapter->flags2 & 
>>> IXGBE_FLAG2_VF_IPSEC_ENABLED)) {
>>>      904                 e_warn(drv, "VF %d attempted to add an IPsec
>>> SA\n", vf);
>>>      905                 err = -EACCES;
>>>      906                 goto err_out;
>>>      907         }
>>>      908
>>>      909         /* Tx IPsec offload doesn't seem to work on this
>>>      910          * device, so block these requests for now.
>>>      911          */
>>>      912         if (sam->dir != XFRM_DEV_OFFLOAD_IN) {
>>>      913                 err = -EOPNOTSUPP;
>>>      914                 goto err_out;
>>>      915         }
>>>      916
>>> --> 917         xs = kzalloc(sizeof(*xs), GFP_KERNEL);
>>>                                            ^^^^^^^^^^
>>> Sleeping allocation.
> 
> what about using GFP_ATOMIC instead of the "default" GFP_KERNEL?
> that would be quickest fix possible, not sure how often such
> alloc would fail

That certainly is another quick option, and may be perfectly reasonable. 
  Can you run with it?

sln

> 
>>>
>>> The call tree that Smatch is worried about is:
>>>
>>> ixgbe_msix_other() <- IRQ handler
>>> -> ixgbe_msg_task()
>>>     -> ixgbe_rcv_msg_from_vf()
>>>        -> ixgbe_ipsec_vf_add_sa()
>>>
>>> This is a fairly new warning and those have a higher risk of false
>>> positives.  Plus the longer the call tree the higher the chance of
>>> false positives.  However, I did review it and the warning looks
>>> reasonable.
>>>
>>> regards,
>>> dan carpenter
>>
>> Hmmm... yes, this does look to be a valid issue.  Nothing like getting
>> haunted by code from the past.  Thanks (?) for digging this up :-) .
> 
> :)
> 
>>
>> I'm not sure offhand what the right answer might be.  I suppose choices
>> include
>>    (a) pre-allocating some number of these xfrm_state structs
>>    (b) shoving the sa creation into a workthread
>>    (c) remove the VF xfrm offload feature
>> Neither of these options seem very appetizing.
>>
>> I would guess that (b) is the "correct" answer, but I don't know how
>> well the PF<->VF mailbox protocol can tolerate the need for a delayed
>> response - it looks like the PF's handler wants to send an immediate
>> ACK/NACK.
>>
>> The pre-allocations for choice (a) would allow for not messing with the
>> timing of the result message, but would require guessing at how many 744
>> byte xfrm_state structs should be lying around for potential use.  The
>> device has 1k slots available, but I don't think we want to store up
>> that many nearly 1k structs that likely won't be used.  Maybe add a
>> switch in the PF for enabling this, which defaults to off?
>>
>> Meanwhile, (c) is the quick and dirty answer for a feature that likely
>> doesn't see much use (I have no data for this assertion, just a guess),
>> and shouldn't be relied upon anyway.
>>
>> I'm not in a position at the moment to be able to address this issue,
>> but I'm happy to try to answer questions for anyone who can get to it.
>> I'm hoping that Jesse, Jake, or Tony might have a better idea what to do
>> with this.
>>
>> Thanks,
>> sln
>>
>