Received-SPF: pass (google.com: domain of linux-kernel+bounces-78967-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <da6a308f-8053-4744-8b81-67b63232a526@uliege.be>
Date: Fri, 23 Feb 2024 19:20:04 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v4 3/3] net: exthdrs: ioam6: send trace event
Content-Language: en-US
To: netdev@vger.kernel.org
Cc: davem@davemloft.net, dsahern@kernel.org, edumazet@google.com,
 kuba@kernel.org, pabeni@redhat.com, linux-kernel@vger.kernel.org,
 justin.iurman@uliege.be
References: <20240223144135.12826-1-justin.iurman@uliege.be>
 <20240223144135.12826-4-justin.iurman@uliege.be>
From: Justin Iurman <justin.iurman@uliege.be>
In-Reply-To: <20240223144135.12826-4-justin.iurman@uliege.be>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2/23/24 15:41, Justin Iurman wrote:
> If we're processing an IOAM Pre-allocated Trace Option-Type (the only
> one supported currently), and if we're the destination, then send the
> trace as an ioam6 event to the multicast group. This way, user space
> apps will be able to collect IOAM data (for a trace, it only makes sense
> to send events if we're the destination).
> 
> Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
> ---
>   net/ipv6/exthdrs.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index 02e9ffb63af1..003f35d9b42b 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c
> @@ -50,6 +50,7 @@
>   #endif
>   #include <net/rpl.h>
>   #include <linux/ioam6.h>
> +#include <linux/ioam6_genl.h>
>   #include <net/ioam6.h>
>   #include <net/dst_metadata.h>
>   
> @@ -954,6 +955,11 @@ static bool ipv6_hop_ioam(struct sk_buff *skb, int optoff)
>   						   + optoff + sizeof(*hdr));
>   
>   		ioam6_fill_trace_data(skb, ns, trace, true);
> +
> +		if (skb_dst(skb)->dev->flags & IFF_LOOPBACK)

Hmm, now that I think about this a bit more, I'm not sure we should 
force a node to be the destination in order to send the event. Instead, 
we could just remove the "am I the destination" check and directly call 
ioam6_event(), which does check if there is at least one listener before 
sending the event. I see two reasons for the change: (a) one may want 
(for whatever reason) to capture IOAM traces on transit nodes, or (b) 
one may use inline (instead of encap) IOAM injection where the 
destination may be outside the IOAM domain (i.e., destination != 
decap/egress IOAM node), which is not RFC8200-compliant and requires 
removing the IOAM Option-Type in-flight from the HbH, but that's another 
story.

Any opinion?

> +			ioam6_event(IOAM6_EVENT_TRACE, dev_net(skb->dev),
> +				    GFP_ATOMIC, (void *)trace,
> +				    hdr->opt_len - 2);
>   		break;
>   	default:
>   		break;