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

[Paolo Abeni <pabeni@redhat.com>]

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@uliege.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?

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