Date: Mon, 8 Apr 2024 15:02:31 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antony Antony <antony@...nome.org>,
	Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc: Antony Antony <antony.antony@...unet.com>,
	Herbert Xu <herbert@...dor.apana.org.au>, netdev@...r.kernel.org,
	devel@...ux-ipsec.org, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v6] xfrm: Add Direction to the
 SA in or out

2024-04-07, 10:23:21 +0200, Antony Antony wrote:
> Hi Sabrina,
> 
> On Fri, Apr 05, 2024 at 11:56:00PM +0200, Sabrina Dubroca via Devel wrote:
> > Hi Antony,
> > 
> > 2024-04-05, 14:40:07 +0200, Antony Antony wrote:
> > > This patch introduces the 'dir' attribute, 'in' or 'out', to the
> > > xfrm_state, SA, enhancing usability by delineating the scope of values
> > > based on direction. An input SA will now exclusively encompass values
> > > pertinent to input, effectively segregating them from output-related
> > > values. 
> > 
> > But this patch isn't doing that for existing properties (I'm thinking
> > of replay window, not sure if any others are relevant [1]). Why not?
> 
> Thank you for raising this point.  I thought that introducing a patch for 
> the replay window check could stir more controversy, which might delay the 
> acceptance of the essential 'dir' feature.

I'm not convinced it's *that* critical. People have someone managed to
use IPsec without it for all those years. Is the intention to only
allow setting up IPTFS SAs when this new 'dir' attribute is provided?
If not, then this patch is not really blocking for IPTFS.

And yes, people will sometimes make comments on patches that cause
delays in getting the patches accepted. Some patches even end up
getting rejected. The kernel is better thanks to that process, even if
it can be annoying to the submitter (including me! it would be a lot
more relaxing if my patches always just went in at v1 :)).

Nicolas, since you were objecting to the informational nature of the
attribute in v5: would you still object to the new attribute (and not
just limited to offload cases) if it properly restricted attributes
that don't match the direction?

>  My primary goal at this stage is 
> to get this basic feature  in and to convince Chris to integrate the "dir" 
> attribute into IP-TFS. This patch has partly contributed to the delays in 
> IP-TFS's development.
> 
> Given your input, I'm curious about the specific conditions you have in 
> mind. See the attached patch.

I didn't look into details when I wrote that email. The rough idea was
"Whatever makes the kernel do replay protection on incoming packets"
(and if any attribute is output-only, skip those on input SAs).

> For non-ESN scenarios, the outbound SA should have a replay window set to 0?

Looks ok.

> And for ESN 1?

Why 1 and not 0?

Does setting xfrm_replay_state_esn->{oseq,oseq_hi} on an input SA (and
{seq,seq_hi} on output) make sense?


And xfrm_state_update probably needs to check that the dir value
matches?  If we get this far we know the new state had matching
direction and properties, but maybe the old one didn't?

In XFRMA_SA_EXTRA_FLAGS, both XFRM_SA_XFLAG_DONT_ENCAP_DSCP and
XFRM_SA_XFLAG_OSEQ_MAY_WRAP look like they're only used on output. A
few of the flags defined with xfrm_usersa_info also seem to work only
in one direction (XFRM_STATE_DECAP_DSCP, XFRM_STATE_NOPMTUDISC,
XFRM_STATE_ICMP).


> non-ESN
> ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2  \
> mode tunnel dir out aead 'rfc4106(gcm(aes))' \
> 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10
> Error: Replay-window too big > 0 for OUT SA.

I'd probably change the string to "Replay window should not be set for
OUT SA", that makes a bit more sense to me. "too big" implies that
some values are valid, which isn't really the case.

> The current impelementation does not replay window 0 with ESN.  Even though 
> disabling replay window with ESN is a desired feature.
> 
> ip xfrm state add src 10.1.3.4 dst 10.1.2.3 proto esp spi 3 reqid 2 mode 
> tunnel dir out flag esn  aead  'rfc4106(gcm(aes))'  \
> 0x2222222222222222222222222222222222222222 96 if_id 11 replay-window 10
> Error: ESN replay-window too big > 1 for OUT SA.ww
> 
> I wonder would the attached patch get accepted quickly.

I'm more interested in getting things right if we're going to
introduce a new bit of API. IMO a new "dir" attribute that doesn't
fully lock down the options on an SA is worse than no attribute (as
Nicolas said previously, it's really confusing).
And I'm not that familiar with all the API for xfrm SAs, so the
properties I listed above may not be everything we should lock down
(and maybe some are wrong).

I think it would also make sense to only accept this attribute in
xfrm_add_sa, and not for any of the other message types. Sharing
xfrma_policy is convenient but not all attributes are valid for all
requests. Old attributes can't be changed, but we should try to be
more strict when we introduce new attributes.

Sorry that I didn't notice this when you posted the previous versions.

-- 
Sabrina

Powered by blists - more mailing lists