netdev - Re: [PATCH ipsec-next v5 06/17] xfrm: add mode

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <m2mslx8kwx.fsf@ja.int.chopps.org>
Date: Wed, 31 Jul 2024 14:32:41 -0400
From: Christian Hopps <chopps@...pps.org>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Christian Hopps <chopps@...pps.org>, devel@...ux-ipsec.org, Steffen
 Klassert <steffen.klassert@...unet.com>, netdev@...r.kernel.org, Christian
 Hopps <chopps@...n.net>
Subject: Re: [PATCH ipsec-next v5 06/17] xfrm: add mode_cbs module
 functionality


Sabrina Dubroca <sd@...asysnail.net> writes:

> 2024-07-30, 17:29:06 -0400, Christian Hopps wrote:
>>
>> Sabrina Dubroca <sd@...asysnail.net> writes:
>>
>> > 2024-07-14, 16:22:34 -0400, Christian Hopps wrote:
>> > > +struct xfrm_mode_cbs {
>> >
>> > It would be nice to add kdoc for the whole thing.
>>
>> Ok, I'll move the inline comments to a kdoc. FWIW, all the other structs in
>> this header, including the main `xfrm_state` struct use the same inline
>> comment documentation style I copied.
>
> Sure, but I don't think we should model new code on old habits.
>
>> > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
>> > > index 7cee9c0a2cdc..6ff05604f973 100644
>> > > --- a/net/xfrm/xfrm_input.c
>> > > +++ b/net/xfrm/xfrm_input.c
>> > > @@ -494,6 +497,10 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>> > >
>> > >  		family = x->props.family;
>> > >
>> > > +		/* An encap_type of -3 indicates reconstructed inner packet */
>> >
>> > And I think it's time to document all the encap_types above the
>> > function (and in particular, how xfrm_inner_mode_input/encap_type=-3
>> > pair together), and/or define some constants. Also, is -2 used
>> > anywhere (I only see -1 and -3)? If not, then why -3?
>>
>> At the time this was added ISTR that there was some belief that -2
>> was used perhaps in an upcoming patch, so I picked -3. I can't find
>> a -2 use case though so I will switch to -2 instead.
>>
>> Re documentation: I think the inline comments where encap_type is
>> used is sufficient documentation for the 2 negative values.
>
> I don't think it is. Inline comments are good to explain the internal
> behavior, but that's more external behavior.

>> There's
>> a lot going on in this function and someone wishing to change (or
>> understand) something is going to have to walk the code and use
>> cases regardless of a bit of extra verbiage on the encap_value
>> beyond what's already there. Fully documenting how xfrm_input works
>> (in all it's use cases) seems beyond the scope of this patch to me.
>
> Sure, and that's really not what I'm asking for here. Something like
> "encap_type=-3 makes xfrm_input jump right back to where it stopped
> when xfrm_inner_mode_input returned -EINPROGRESS" is useful without
> having to dive into the mess that is xfrm_input.

If I'm not adding your suggested text into an inline comment where am I doing this?

Bear in mind that encap_type can also have non-negative values, am I documenting all these cases too? It just seems like going down this path is asking for the entire function to be documented, perhaps I'm missing something though.

Are other people going to be OK with a top of function comment that only documents the single (now) `-2` value for encap_type?

Thanks,
Chris.