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]
Message-ID: <5bbba416-9c98-47f3-88b5-66747998bba5@uliege.be>
Date: Tue, 13 Aug 2024 13:43:28 +0200
From: Justin Iurman <justin.iurman@...ege.be>
To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
 kuba@...nel.org, linux-kernel@...r.kernel.org, justin.iurman@...ege.be
Subject: Re: [PATCH net-next 2/2] net: ipv6: ioam6: new feature tunsrc

On 8/13/24 13:06, Paolo Abeni wrote:
> On 8/9/24 14:39, Justin Iurman wrote:
>> This patch provides a new feature (i.e., "tunsrc") for the tunnel (i.e.,
>> "encap") mode of ioam6. Just like seg6 already does, except it is
>> attached to a route. The "tunsrc" is optional: when not provided (by
>> default), the automatic resolution is applied. Using "tunsrc" when
>> possible has a benefit: performance.
> 
> It's customary to include performances figures in performance related 
> changeset ;)
> 
>>
>> Signed-off-by: Justin Iurman <justin.iurman@...ege.be>
>> ---
>>   include/uapi/linux/ioam6_iptunnel.h |  7 +++++
>>   net/ipv6/ioam6_iptunnel.c           | 48 ++++++++++++++++++++++++++---
>>   2 files changed, 51 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/uapi/linux/ioam6_iptunnel.h 
>> b/include/uapi/linux/ioam6_iptunnel.h
>> index 38f6a8fdfd34..6cdbd0da7ad8 100644
>> --- a/include/uapi/linux/ioam6_iptunnel.h
>> +++ b/include/uapi/linux/ioam6_iptunnel.h
>> @@ -50,6 +50,13 @@ enum {
>>       IOAM6_IPTUNNEL_FREQ_K,        /* u32 */
>>       IOAM6_IPTUNNEL_FREQ_N,        /* u32 */
>> +    /* Tunnel src address.
>> +     * For encap,auto modes.
>> +     * Optional (automatic if
>> +     * not provided).
>> +     */
>> +    IOAM6_IPTUNNEL_SRC,        /* struct in6_addr */
>> +
>>       __IOAM6_IPTUNNEL_MAX,
>>   };
>> diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
>> index cd2522f04edf..e0e73faf9969 100644
>> --- a/net/ipv6/ioam6_iptunnel.c
>> +++ b/net/ipv6/ioam6_iptunnel.c
>> @@ -42,6 +42,8 @@ struct ioam6_lwt {
>>       struct ioam6_lwt_freq freq;
>>       atomic_t pkt_cnt;
>>       u8 mode;
>> +    bool has_tunsrc;
>> +    struct in6_addr tunsrc;
>>       struct in6_addr tundst;
>>       struct ioam6_lwt_encap tuninfo;
>>   };
>> @@ -72,6 +74,7 @@ static const struct nla_policy 
>> ioam6_iptunnel_policy[IOAM6_IPTUNNEL_MAX + 1] = {
>>       [IOAM6_IPTUNNEL_MODE]    = NLA_POLICY_RANGE(NLA_U8,
>>                              IOAM6_IPTUNNEL_MODE_MIN,
>>                              IOAM6_IPTUNNEL_MODE_MAX),
>> +    [IOAM6_IPTUNNEL_SRC]    = NLA_POLICY_EXACT_LEN(sizeof(struct 
>> in6_addr)),
>>       [IOAM6_IPTUNNEL_DST]    = NLA_POLICY_EXACT_LEN(sizeof(struct 
>> in6_addr)),
>>       [IOAM6_IPTUNNEL_TRACE]    = NLA_POLICY_EXACT_LEN(
>>                       sizeof(struct ioam6_trace_hdr)),
>> @@ -144,6 +147,11 @@ static int ioam6_build_state(struct net *net, 
>> struct nlattr *nla,
>>       else
>>           mode = nla_get_u8(tb[IOAM6_IPTUNNEL_MODE]);
>> +    if (tb[IOAM6_IPTUNNEL_SRC] && mode == IOAM6_IPTUNNEL_MODE_INLINE) {
>> +        NL_SET_ERR_MSG(extack, "no tunnel source expected in this 
>> mode");
>> +        return -EINVAL;
>> +    }
> 
> when mode is IOAM6_IPTUNNEL_MODE_AUTO, the data path could still add the 
> encapsulation for forwarded packets, why explicitly preventing this 
> optimization in such scenario?

Actually, this check is correct. We don't want the "tunsrc" with 
"inline" mode since it's useless. If the "auto" mode is chosen, then 
it's fine (same for the "encap" mode). Preventing "tunsrc" for the 
"inline" mode does *not* impact the auto mode. Basically:

tb[IOAM6_IPTUNNEL_SRC] && mode == IOAM6_IPTUNNEL_MODE_INLINE -> error
tb[IOAM6_IPTUNNEL_SRC] && mode == IOAM6_IPTUNNEL_MODE_ENCAP -> OK
tb[IOAM6_IPTUNNEL_SRC] && mode == IOAM6_IPTUNNEL_MODE_AUTO -> OK

It aligns better with the semantics of "tundst", which is a MUST for 
"encap"/"auto" modes (and forbidden for the "inline" mode). In the case 
of "tunsrc", it is a MAY for "encap"/"auto" modes (and forbidden for the 
"inline" mode).

>> +
>>       if (!tb[IOAM6_IPTUNNEL_DST] && mode != 
>> IOAM6_IPTUNNEL_MODE_INLINE) {
>>           NL_SET_ERR_MSG(extack, "this mode needs a tunnel destination");
>>           return -EINVAL;
>> @@ -178,6 +186,14 @@ static int ioam6_build_state(struct net *net, 
>> struct nlattr *nla,
>>       ilwt->freq.n = freq_n;
>>       ilwt->mode = mode;
>> +
>> +    if (!tb[IOAM6_IPTUNNEL_SRC]) {
>> +        ilwt->has_tunsrc = false;
>> +    } else {
>> +        ilwt->has_tunsrc = true;
>> +        ilwt->tunsrc = nla_get_in6_addr(tb[IOAM6_IPTUNNEL_SRC]);
> 
> Since you are going to use the source address only if != ANY, I think it 
> would be cleaner to refuse such addresses here. That will avoid an 
> additional check in the datapath.
> 
> Cheers,
> 
> Paolo
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ