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

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
To: Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Jacob Keller <jacob.e.keller@intel.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: intel-wired-lan@lists.osuosl.org,
	Dan Carpenter <dan.carpenter@linaro.org>,
	"Nelson, Shannon" <shannon.nelson@amd.com>
Subject: Re: [Intel-wired-lan] [bug report] ixgbe: add VF IPsec management
Date: Wed, 14 Feb 2024 14:58:48 +0100	[thread overview]
Message-ID: <93ed20ec-848e-4c72-8c01-e47acd4e1d8f@intel.com> (raw)
In-Reply-To: <b5b28ce2-4322-4d39-93ac-46d32bb336fe@amd.com>

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

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