netdev - Re: [PATCH net-next v2 2/3] nfp: add framework to support ipsec offloading

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 13 Oct 2022 16:21:54 +0800
From:   Yinjun Zhang <yinjun.zhang@...igine.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Cc: chengtian.liu@...igine.com, ;
        Simon Horman <simon.horman@...igine.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        oss-drivers@...igine.com,
        Huanhuan Wang <huanhuan.wang@...igine.com>
Subject: Re: [PATCH net-next v2 2/3] nfp: add framework to support ipsec
 offloading

On Tue, Oct 11, 2022 at 11:41:47AM +0300, Leon Romanovsky wrote:
> > 
> > `sa_free_stack` is used to maintain the used/available sa entries, which
> > is initialized in `nfp_net_ipsec_init`.
> > Yes, it's indeed a big array, and we're going to use pointer instead of array
> > here.
> 
> Why do you want to use array and not Xarray?
> 

We'll try that. Thanks.

> > 
> > > > +bool nfp_net_ipsec_tx_prep(struct nfp_net_dp *dp, struct sk_buff *skb,
> > > > +			   struct nfp_ipsec_offload *offload_info)
> > > > +{
> > > > +	struct xfrm_offload *xo = xfrm_offload(skb);
> > > > +	struct xfrm_state *x;
> > > > +
> > > > +	if (!xo)
> > > > +		return false;
> > > 
> > > How is it possible in offload path?
> > > Why do all drivers check sec_path length and not xo?
> > > 
> > 
> > `tx_prep` is called in the tx datapath, we use `xo` to check if the
> > packet needs offload-encrypto or not.
> 
> You didn't answer on any of my questions above.
> 
> How is it possible in offload path?
> Why do all drivers check sec_path length and not xo?
> 

It's not a offload-only path, but a normal tx data path. Only if xo is
not NULL, we're going to do crypto-offload. Not every transimitted
packet needs crypto, right? 
We're going to move this check to its parent function to avoid
unnecessary jump when it's not a to-crypto packet.

> > 
> > > > +	saidx = meta->ipsec_saidx - 1;
> > > > +	if (saidx > NFP_NET_IPSEC_MAX_SA_CNT || saidx < 0) {
> > > > +		nn_dp_warn(dp, "Invalid SAIDX from NIC %d\n", saidx);
> > > 
> > > No prints in data path that can be triggered from the network, please.
> > > 
> > 
> > It's a ratelimit print, and it means severe error happens, probably
> > unrecoverable, when running into this path.
> 
> The main part of the sentence is "... can be triggered from the network ..."

OK, we'll remove this print, and maybe introduce some error counters instead in
following patch series.

> 
> Thanks