Re: [RFC ipsec-next v2 2/8] iptfs: uapi: ip: add ip_tfs_*_hdr packet formats

[Sabrina Dubroca <sd@queasysnail.net>]

2023-11-20, 15:18:49 -0500, Christian Hopps wrote:
> 
> Sabrina Dubroca <sd@queasysnail.net> writes:
> 
> > 2023-11-12, 22:52:13 -0500, Christian Hopps wrote:
> > > From: Christian Hopps <chopps@labn.net>
> > > 
> > > Add the on-wire basic and congestion-control IP-TFS packet headers.
> > > 
> > > Signed-off-by: Christian Hopps <chopps@labn.net>
> > > ---
> > >  include/uapi/linux/ip.h | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/include/uapi/linux/ip.h b/include/uapi/linux/ip.h
> > > index 283dec7e3645..cc83878ecf08 100644
> > > --- a/include/uapi/linux/ip.h
> > > +++ b/include/uapi/linux/ip.h
> > > @@ -137,6 +137,23 @@ struct ip_beet_phdr {
> > >  	__u8 reserved;
> > >  };
> > > 
> > > +struct ip_iptfs_hdr {
> > > +	__u8 subtype;		/* 0*: basic, 1: CC */
> > > +	__u8 flags;
> > > +	__be16 block_offset;
> > > +};
> > > +
> > > +struct ip_iptfs_cc_hdr {
> > > +	__u8 subtype;		/* 0: basic, 1*: CC */
> > > +	__u8 flags;
> > > +	__be16 block_offset;
> > > +	__be32 loss_rate;
> > > +	__u8 rtt_and_adelay1[4];
> > > +	__u8 adelay2_and_xdelay[4];
> > 
> > Given how the fields are split, wouldn't it be more convenient to have
> > a single __be64, rather than reading some bits from multiple __u8?
> 
> This is a good point, I carried this over from an earlier implementation, let me give it some though but probably change it.
> 
> > > +	__be32 tval;
> > > +	__be32 techo;
> > > +};
> 
> > I don't think these need to be part of uapi. Can we move them to
> > include/net/iptfs.h (or possibly net/xfrm/xfrm_iptfs.c)? It would also
> > make more sense to have them near the definitions for
> > IPTFS_SUBTYPE_*. And it would be easier to change how we split and
> > name fields for kernel consumption if we're not stuck with whatever we
> > put in uapi.
> 
> The other ipsec modes headers were added here, so I was simply
> following along. I don't mind moving things but would like to
> understand why iptfs would be different from the other modes, for
> example, `struct ip_comp_hdr` and `struct ip_beet_phdr` appears in
> this file.

IMHO it was a mistake that was made when include/uapi was created,
they should not have been part of uapi since there's never an exchange
between kernel and userspace using those. I guess it's less
problematic because the header formats are simple (everything fits
nicely into a u8/u16/u32) and they were already used in the kernel for
a long time so pretty much stable (ie no doubt about whether the
split and naming that was chosen was right).

But if others feel strongly about putting those structs in uapi, I can
live with that.

I'll send comments on the rest of the series as I continue making my
way through it. The main patch is going to take me a while :(

-- 
Sabrina