linux-kernel - Re: [PATCH net-next 2/2] net: ipv6: ioam6: new feature tunsrc

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: <bf82e839-585d-47c6-822e-f994d9c92389@uliege.be>
Date: Tue, 13 Aug 2024 13:20:40 +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 ;)

Indeed, I realized it too late... thx for the reminder!

Before (= "encap" mode): https://ibb.co/bNCzvf7
After (= "encap" mode with "tunsrc"): https://ibb.co/PT8L6yq

I'll add these again to -v2.

>> 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?

Good catch! I guess we can just ignore "tunsrc" if provided with inline 
mode, instead of returning an error.

>> +
>>       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.

+1.

Thanks,
Justin

> Cheers,
> 
> Paolo
>