New function dev_fill_bridge_path(), similar to dev_fill_forward_path().
It handles starting from a bridge port instead of the bridge master.
The structures ctx and nft_forward_info need to be already filled in with
the (vlan) encaps.

Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
---
 include/linux/netdevice.h |  2 +
 net/core/dev.c            | 77 ++++++++++++++++++++++++++++++++-------
 2 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e87b5e488325..9d80f650345e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3069,6 +3069,8 @@ void dev_remove_offload(struct packet_offload *po);
 
 int dev_get_iflink(const struct net_device *dev);
 int dev_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb);
+int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
+			 struct net_device_path_stack *stack);
 int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
 			  struct net_device_path_stack *stack);
 struct net_device *__dev_get_by_flags(struct net *net, unsigned short flags,
diff --git a/net/core/dev.c b/net/core/dev.c
index cd479f5f22f6..49959c4904fc 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -713,44 +713,95 @@ static struct net_device_path *dev_fwd_path(struct net_device_path_stack *stack)
 	return &stack->path[k];
 }
 
-int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
-			  struct net_device_path_stack *stack)
+static int dev_fill_forward_path_common(struct net_device_path_ctx *ctx,
+					struct net_device_path_stack *stack)
 {
 	const struct net_device *last_dev;
-	struct net_device_path_ctx ctx = {
-		.dev	= dev,
-	};
 	struct net_device_path *path;
 	int ret = 0;
 
-	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
-	stack->num_paths = 0;
-	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
-		last_dev = ctx.dev;
+	while (ctx->dev && ctx->dev->netdev_ops->ndo_fill_forward_path) {
+		last_dev = ctx->dev;
 		path = dev_fwd_path(stack);
 		if (!path)
 			return -1;
 
 		memset(path, 0, sizeof(struct net_device_path));
-		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
+		ret = ctx->dev->netdev_ops->ndo_fill_forward_path(ctx, path);
 		if (ret < 0)
 			return -1;
 
-		if (WARN_ON_ONCE(last_dev == ctx.dev))
+		if (WARN_ON_ONCE(last_dev == ctx->dev))
 			return -1;
 	}
 
-	if (!ctx.dev)
+	if (!ctx->dev)
 		return ret;
 
 	path = dev_fwd_path(stack);
 	if (!path)
 		return -1;
 	path->type = DEV_PATH_ETHERNET;
-	path->dev = ctx.dev;
+	path->dev = ctx->dev;
+
+	return ret;
+}
+
+int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
+			 struct net_device_path_stack *stack)
+{
+	const struct net_device *last_dev, *br_dev;
+	struct net_device_path *path;
+	int ret = 0;
+
+	stack->num_paths = 0;
+
+	if (!ctx->dev || !netif_is_bridge_port(ctx->dev))
+		return -1;
+
+	br_dev = netdev_master_upper_dev_get_rcu((struct net_device *)ctx->dev);
+	if (!br_dev || !br_dev->netdev_ops->ndo_fill_forward_path)
+		return -1;
+
+	last_dev = ctx->dev;
+	path = dev_fwd_path(stack);
+	if (!path)
+		return -1;
+
+	memset(path, 0, sizeof(struct net_device_path));
+	ret = br_dev->netdev_ops->ndo_fill_forward_path(ctx, path);
+	if (ret < 0)
+		return -1;
+
+	if (!ctx->dev || WARN_ON_ONCE(last_dev == ctx->dev))
+		return -1;
+
+	if (!netif_is_bridge_master(ctx->dev))
+		return dev_fill_forward_path_common(ctx, stack);
+
+	path = dev_fwd_path(stack);
+	if (!path)
+		return -1;
+	path->type = DEV_PATH_ETHERNET;
+	path->dev = ctx->dev;
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(dev_fill_bridge_path);
+
+int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
+			  struct net_device_path_stack *stack)
+{
+	struct net_device_path_ctx ctx = {
+		.dev	= dev,
+	};
+
+	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
+
+	stack->num_paths = 0;
+
+	return dev_fill_forward_path_common(&ctx, stack);
+}
 EXPORT_SYMBOL_GPL(dev_fill_forward_path);
 
 /**
-- 
2.45.2

On 13/10/2024 21:55, Eric Woudstra wrote:
> New function dev_fill_bridge_path(), similar to dev_fill_forward_path().
> It handles starting from a bridge port instead of the bridge master.
> The structures ctx and nft_forward_info need to be already filled in with
> the (vlan) encaps.
> 
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> ---
>  include/linux/netdevice.h |  2 +
>  net/core/dev.c            | 77 ++++++++++++++++++++++++++++++++-------
>  2 files changed, 66 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index e87b5e488325..9d80f650345e 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -3069,6 +3069,8 @@ void dev_remove_offload(struct packet_offload *po);
>  
>  int dev_get_iflink(const struct net_device *dev);
>  int dev_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb);
> +int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
> +			 struct net_device_path_stack *stack);
>  int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>  			  struct net_device_path_stack *stack);
>  struct net_device *__dev_get_by_flags(struct net *net, unsigned short flags,
> diff --git a/net/core/dev.c b/net/core/dev.c
> index cd479f5f22f6..49959c4904fc 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -713,44 +713,95 @@ static struct net_device_path *dev_fwd_path(struct net_device_path_stack *stack)
>  	return &stack->path[k];
>  }
>  
> -int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
> -			  struct net_device_path_stack *stack)
> +static int dev_fill_forward_path_common(struct net_device_path_ctx *ctx,
> +					struct net_device_path_stack *stack)
>  {
>  	const struct net_device *last_dev;
> -	struct net_device_path_ctx ctx = {
> -		.dev	= dev,
> -	};
>  	struct net_device_path *path;
>  	int ret = 0;
>  
> -	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
> -	stack->num_paths = 0;
> -	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
> -		last_dev = ctx.dev;
> +	while (ctx->dev && ctx->dev->netdev_ops->ndo_fill_forward_path) {
> +		last_dev = ctx->dev;
>  		path = dev_fwd_path(stack);
>  		if (!path)
>  			return -1;
>  
>  		memset(path, 0, sizeof(struct net_device_path));
> -		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
> +		ret = ctx->dev->netdev_ops->ndo_fill_forward_path(ctx, path);
>  		if (ret < 0)
>  			return -1;
>  
> -		if (WARN_ON_ONCE(last_dev == ctx.dev))
> +		if (WARN_ON_ONCE(last_dev == ctx->dev))
>  			return -1;
>  	}
>  
> -	if (!ctx.dev)
> +	if (!ctx->dev)
>  		return ret;
>  
>  	path = dev_fwd_path(stack);
>  	if (!path)
>  		return -1;
>  	path->type = DEV_PATH_ETHERNET;
> -	path->dev = ctx.dev;
> +	path->dev = ctx->dev;
> +
> +	return ret;
> +}
> +
> +int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
> +			 struct net_device_path_stack *stack)
> +{
> +	const struct net_device *last_dev, *br_dev;
> +	struct net_device_path *path;
> +	int ret = 0;
> +
> +	stack->num_paths = 0;
> +
> +	if (!ctx->dev || !netif_is_bridge_port(ctx->dev))
> +		return -1;
> +
> +	br_dev = netdev_master_upper_dev_get_rcu((struct net_device *)ctx->dev);
> +	if (!br_dev || !br_dev->netdev_ops->ndo_fill_forward_path)
> +		return -1;
> +
> +	last_dev = ctx->dev;
> +	path = dev_fwd_path(stack);
> +	if (!path)
> +		return -1;
> +
> +	memset(path, 0, sizeof(struct net_device_path));
> +	ret = br_dev->netdev_ops->ndo_fill_forward_path(ctx, path);
> +	if (ret < 0)
> +		return -1;
> +
> +	if (!ctx->dev || WARN_ON_ONCE(last_dev == ctx->dev))
> +		return -1;
> +
> +	if (!netif_is_bridge_master(ctx->dev))

hmm, do we expect ctx->dev to be a bridge master? Looking at
br_fill_forward_path, it seems to be == fdb->dst->dev which
should be the target port

> +		return dev_fill_forward_path_common(ctx, stack);
> +
> +	path = dev_fwd_path(stack);
> +	if (!path)
> +		return -1;
> +	path->type = DEV_PATH_ETHERNET;
> +	path->dev = ctx->dev;
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(dev_fill_bridge_path);
> +
> +int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
> +			  struct net_device_path_stack *stack)
> +{
> +	struct net_device_path_ctx ctx = {
> +		.dev	= dev,
> +	};
> +
> +	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
> +
> +	stack->num_paths = 0;
> +
> +	return dev_fill_forward_path_common(&ctx, stack);
> +}
>  EXPORT_SYMBOL_GPL(dev_fill_forward_path);
>  
>  /**

On 10/14/24 8:59 AM, Nikolay Aleksandrov wrote:
> On 13/10/2024 21:55, Eric Woudstra wrote:
>> New function dev_fill_bridge_path(), similar to dev_fill_forward_path().
>> It handles starting from a bridge port instead of the bridge master.
>> The structures ctx and nft_forward_info need to be already filled in with
>> the (vlan) encaps.
>>
>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>> ---
>>  include/linux/netdevice.h |  2 +
>>  net/core/dev.c            | 77 ++++++++++++++++++++++++++++++++-------
>>  2 files changed, 66 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index e87b5e488325..9d80f650345e 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -3069,6 +3069,8 @@ void dev_remove_offload(struct packet_offload *po);
>>  
>>  int dev_get_iflink(const struct net_device *dev);
>>  int dev_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb);
>> +int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
>> +			 struct net_device_path_stack *stack);
>>  int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>>  			  struct net_device_path_stack *stack);
>>  struct net_device *__dev_get_by_flags(struct net *net, unsigned short flags,
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index cd479f5f22f6..49959c4904fc 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -713,44 +713,95 @@ static struct net_device_path *dev_fwd_path(struct net_device_path_stack *stack)
>>  	return &stack->path[k];
>>  }
>>  
>> -int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>> -			  struct net_device_path_stack *stack)
>> +static int dev_fill_forward_path_common(struct net_device_path_ctx *ctx,
>> +					struct net_device_path_stack *stack)
>>  {
>>  	const struct net_device *last_dev;
>> -	struct net_device_path_ctx ctx = {
>> -		.dev	= dev,
>> -	};
>>  	struct net_device_path *path;
>>  	int ret = 0;
>>  
>> -	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
>> -	stack->num_paths = 0;
>> -	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
>> -		last_dev = ctx.dev;
>> +	while (ctx->dev && ctx->dev->netdev_ops->ndo_fill_forward_path) {
>> +		last_dev = ctx->dev;
>>  		path = dev_fwd_path(stack);
>>  		if (!path)
>>  			return -1;
>>  
>>  		memset(path, 0, sizeof(struct net_device_path));
>> -		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
>> +		ret = ctx->dev->netdev_ops->ndo_fill_forward_path(ctx, path);
>>  		if (ret < 0)
>>  			return -1;
>>  
>> -		if (WARN_ON_ONCE(last_dev == ctx.dev))
>> +		if (WARN_ON_ONCE(last_dev == ctx->dev))
>>  			return -1;
>>  	}
>>  
>> -	if (!ctx.dev)
>> +	if (!ctx->dev)
>>  		return ret;
>>  
>>  	path = dev_fwd_path(stack);
>>  	if (!path)
>>  		return -1;
>>  	path->type = DEV_PATH_ETHERNET;
>> -	path->dev = ctx.dev;
>> +	path->dev = ctx->dev;
>> +
>> +	return ret;
>> +}
>> +
>> +int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
>> +			 struct net_device_path_stack *stack)
>> +{
>> +	const struct net_device *last_dev, *br_dev;
>> +	struct net_device_path *path;
>> +	int ret = 0;
>> +
>> +	stack->num_paths = 0;
>> +
>> +	if (!ctx->dev || !netif_is_bridge_port(ctx->dev))
>> +		return -1;
>> +
>> +	br_dev = netdev_master_upper_dev_get_rcu((struct net_device *)ctx->dev);
>> +	if (!br_dev || !br_dev->netdev_ops->ndo_fill_forward_path)
>> +		return -1;
>> +
>> +	last_dev = ctx->dev;
>> +	path = dev_fwd_path(stack);
>> +	if (!path)
>> +		return -1;
>> +
>> +	memset(path, 0, sizeof(struct net_device_path));
>> +	ret = br_dev->netdev_ops->ndo_fill_forward_path(ctx, path);
>> +	if (ret < 0)
>> +		return -1;
>> +
>> +	if (!ctx->dev || WARN_ON_ONCE(last_dev == ctx->dev))
>> +		return -1;
>> +
>> +	if (!netif_is_bridge_master(ctx->dev))
> 
> hmm, do we expect ctx->dev to be a bridge master? Looking at
> br_fill_forward_path, it seems to be == fdb->dst->dev which
> should be the target port

It would indeed be very unlikely. It was a left-over from code I wrote,
thinking that here I could handle cascaded bridges (via vlan-device). I
dropped that, since conntrack does not follow this flow.

So would it be better to only make sure that ctx->dev is not a bridge
master?

	if (netif_is_bridge_master(ctx->dev))
		return -1;

	return dev_fill_forward_path_common(ctx, stack);

>> +		return dev_fill_forward_path_common(ctx, stack);
>> +
>> +	path = dev_fwd_path(stack);
>> +	if (!path)
>> +		return -1;
>> +	path->type = DEV_PATH_ETHERNET;
>> +	path->dev = ctx->dev;
>>  
>>  	return ret;
>>  }
>> +EXPORT_SYMBOL_GPL(dev_fill_bridge_path);
>> +
>> +int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>> +			  struct net_device_path_stack *stack)
>> +{
>> +	struct net_device_path_ctx ctx = {
>> +		.dev	= dev,
>> +	};
>> +
>> +	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
>> +
>> +	stack->num_paths = 0;
>> +
>> +	return dev_fill_forward_path_common(&ctx, stack);
>> +}
>>  EXPORT_SYMBOL_GPL(dev_fill_forward_path);
>>  
>>  /**
>

On 14/10/2024 21:34, Eric Woudstra wrote:
> 
> 
> On 10/14/24 8:59 AM, Nikolay Aleksandrov wrote:
>> On 13/10/2024 21:55, Eric Woudstra wrote:
>>> New function dev_fill_bridge_path(), similar to dev_fill_forward_path().
>>> It handles starting from a bridge port instead of the bridge master.
>>> The structures ctx and nft_forward_info need to be already filled in with
>>> the (vlan) encaps.
>>>
>>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>>> ---
>>>  include/linux/netdevice.h |  2 +
>>>  net/core/dev.c            | 77 ++++++++++++++++++++++++++++++++-------
>>>  2 files changed, 66 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>> index e87b5e488325..9d80f650345e 100644
>>> --- a/include/linux/netdevice.h
>>> +++ b/include/linux/netdevice.h
>>> @@ -3069,6 +3069,8 @@ void dev_remove_offload(struct packet_offload *po);
>>>  
>>>  int dev_get_iflink(const struct net_device *dev);
>>>  int dev_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb);
>>> +int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
>>> +			 struct net_device_path_stack *stack);
>>>  int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>>>  			  struct net_device_path_stack *stack);
>>>  struct net_device *__dev_get_by_flags(struct net *net, unsigned short flags,
>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>> index cd479f5f22f6..49959c4904fc 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -713,44 +713,95 @@ static struct net_device_path *dev_fwd_path(struct net_device_path_stack *stack)
>>>  	return &stack->path[k];
>>>  }
>>>  
>>> -int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>>> -			  struct net_device_path_stack *stack)
>>> +static int dev_fill_forward_path_common(struct net_device_path_ctx *ctx,
>>> +					struct net_device_path_stack *stack)
>>>  {
>>>  	const struct net_device *last_dev;
>>> -	struct net_device_path_ctx ctx = {
>>> -		.dev	= dev,
>>> -	};
>>>  	struct net_device_path *path;
>>>  	int ret = 0;
>>>  
>>> -	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
>>> -	stack->num_paths = 0;
>>> -	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
>>> -		last_dev = ctx.dev;
>>> +	while (ctx->dev && ctx->dev->netdev_ops->ndo_fill_forward_path) {
>>> +		last_dev = ctx->dev;
>>>  		path = dev_fwd_path(stack);
>>>  		if (!path)
>>>  			return -1;
>>>  
>>>  		memset(path, 0, sizeof(struct net_device_path));
>>> -		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
>>> +		ret = ctx->dev->netdev_ops->ndo_fill_forward_path(ctx, path);
>>>  		if (ret < 0)
>>>  			return -1;
>>>  
>>> -		if (WARN_ON_ONCE(last_dev == ctx.dev))
>>> +		if (WARN_ON_ONCE(last_dev == ctx->dev))
>>>  			return -1;
>>>  	}
>>>  
>>> -	if (!ctx.dev)
>>> +	if (!ctx->dev)
>>>  		return ret;
>>>  
>>>  	path = dev_fwd_path(stack);
>>>  	if (!path)
>>>  		return -1;
>>>  	path->type = DEV_PATH_ETHERNET;
>>> -	path->dev = ctx.dev;
>>> +	path->dev = ctx->dev;
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
>>> +			 struct net_device_path_stack *stack)
>>> +{
>>> +	const struct net_device *last_dev, *br_dev;
>>> +	struct net_device_path *path;
>>> +	int ret = 0;
>>> +
>>> +	stack->num_paths = 0;
>>> +
>>> +	if (!ctx->dev || !netif_is_bridge_port(ctx->dev))
>>> +		return -1;
>>> +
>>> +	br_dev = netdev_master_upper_dev_get_rcu((struct net_device *)ctx->dev);
>>> +	if (!br_dev || !br_dev->netdev_ops->ndo_fill_forward_path)
>>> +		return -1;
>>> +
>>> +	last_dev = ctx->dev;
>>> +	path = dev_fwd_path(stack);
>>> +	if (!path)
>>> +		return -1;
>>> +
>>> +	memset(path, 0, sizeof(struct net_device_path));
>>> +	ret = br_dev->netdev_ops->ndo_fill_forward_path(ctx, path);
>>> +	if (ret < 0)
>>> +		return -1;
>>> +
>>> +	if (!ctx->dev || WARN_ON_ONCE(last_dev == ctx->dev))
>>> +		return -1;

* ^^^^^^^^^ here

>>> +
>>> +	if (!netif_is_bridge_master(ctx->dev))
>>
>> hmm, do we expect ctx->dev to be a bridge master? Looking at
>> br_fill_forward_path, it seems to be == fdb->dst->dev which
>> should be the target port
> 
> It would indeed be very unlikely. It was a left-over from code I wrote,
> thinking that here I could handle cascaded bridges (via vlan-device). I
> dropped that, since conntrack does not follow this flow.
> 
> So would it be better to only make sure that ctx->dev is not a bridge
> master?
> 
> 	if (netif_is_bridge_master(ctx->dev))
> 		return -1;
> 
> 	return dev_fill_forward_path_common(ctx, stack);
> 

I think you misunderstood me, I don't think ctx->dev can ever be a bridge
device because ctx->dev gets set to fdb->dst and fdbs that point to the bridge
itself have fdb->dst == NULL but ctx->dev is checked against NULL earlier*
so the bridge dev check doesn't make sense to me.

>>> +		return dev_fill_forward_path_common(ctx, stack);
>>> +
>>> +	path = dev_fwd_path(stack);
>>> +	if (!path)
>>> +		return -1;
>>> +	path->type = DEV_PATH_ETHERNET;
>>> +	path->dev = ctx->dev;
>>>  
>>>  	return ret;
>>>  }
>>> +EXPORT_SYMBOL_GPL(dev_fill_bridge_path);
>>> +
>>> +int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>>> +			  struct net_device_path_stack *stack)
>>> +{
>>> +	struct net_device_path_ctx ctx = {
>>> +		.dev	= dev,
>>> +	};
>>> +
>>> +	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
>>> +
>>> +	stack->num_paths = 0;
>>> +
>>> +	return dev_fill_forward_path_common(&ctx, stack);
>>> +}
>>>  EXPORT_SYMBOL_GPL(dev_fill_forward_path);
>>>  
>>>  /**
>>

On 10/16/24 9:43 AM, Nikolay Aleksandrov wrote:
> On 14/10/2024 21:34, Eric Woudstra wrote:
>>
>>
>> On 10/14/24 8:59 AM, Nikolay Aleksandrov wrote:
>>> On 13/10/2024 21:55, Eric Woudstra wrote:
>>>> New function dev_fill_bridge_path(), similar to dev_fill_forward_path().
>>>> It handles starting from a bridge port instead of the bridge master.
>>>> The structures ctx and nft_forward_info need to be already filled in with
>>>> the (vlan) encaps.
>>>>
>>>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>>>> ---
>>>>  include/linux/netdevice.h |  2 +
>>>>  net/core/dev.c            | 77 ++++++++++++++++++++++++++++++++-------
>>>>  2 files changed, 66 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index e87b5e488325..9d80f650345e 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>>> @@ -3069,6 +3069,8 @@ void dev_remove_offload(struct packet_offload *po);
>>>>  
>>>>  int dev_get_iflink(const struct net_device *dev);
>>>>  int dev_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb);
>>>> +int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
>>>> +			 struct net_device_path_stack *stack);
>>>>  int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>>>>  			  struct net_device_path_stack *stack);
>>>>  struct net_device *__dev_get_by_flags(struct net *net, unsigned short flags,
>>>> diff --git a/net/core/dev.c b/net/core/dev.c
>>>> index cd479f5f22f6..49959c4904fc 100644
>>>> --- a/net/core/dev.c
>>>> +++ b/net/core/dev.c
>>>> @@ -713,44 +713,95 @@ static struct net_device_path *dev_fwd_path(struct net_device_path_stack *stack)
>>>>  	return &stack->path[k];
>>>>  }
>>>>  
>>>> -int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>>>> -			  struct net_device_path_stack *stack)
>>>> +static int dev_fill_forward_path_common(struct net_device_path_ctx *ctx,
>>>> +					struct net_device_path_stack *stack)
>>>>  {
>>>>  	const struct net_device *last_dev;
>>>> -	struct net_device_path_ctx ctx = {
>>>> -		.dev	= dev,
>>>> -	};
>>>>  	struct net_device_path *path;
>>>>  	int ret = 0;
>>>>  
>>>> -	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
>>>> -	stack->num_paths = 0;
>>>> -	while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
>>>> -		last_dev = ctx.dev;
>>>> +	while (ctx->dev && ctx->dev->netdev_ops->ndo_fill_forward_path) {
>>>> +		last_dev = ctx->dev;
>>>>  		path = dev_fwd_path(stack);
>>>>  		if (!path)
>>>>  			return -1;
>>>>  
>>>>  		memset(path, 0, sizeof(struct net_device_path));
>>>> -		ret = ctx.dev->netdev_ops->ndo_fill_forward_path(&ctx, path);
>>>> +		ret = ctx->dev->netdev_ops->ndo_fill_forward_path(ctx, path);
>>>>  		if (ret < 0)
>>>>  			return -1;
>>>>  
>>>> -		if (WARN_ON_ONCE(last_dev == ctx.dev))
>>>> +		if (WARN_ON_ONCE(last_dev == ctx->dev))
>>>>  			return -1;
>>>>  	}
>>>>  
>>>> -	if (!ctx.dev)
>>>> +	if (!ctx->dev)
>>>>  		return ret;
>>>>  
>>>>  	path = dev_fwd_path(stack);
>>>>  	if (!path)
>>>>  		return -1;
>>>>  	path->type = DEV_PATH_ETHERNET;
>>>> -	path->dev = ctx.dev;
>>>> +	path->dev = ctx->dev;
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +int dev_fill_bridge_path(struct net_device_path_ctx *ctx,
>>>> +			 struct net_device_path_stack *stack)
>>>> +{
>>>> +	const struct net_device *last_dev, *br_dev;
>>>> +	struct net_device_path *path;
>>>> +	int ret = 0;
>>>> +
>>>> +	stack->num_paths = 0;
>>>> +
>>>> +	if (!ctx->dev || !netif_is_bridge_port(ctx->dev))
>>>> +		return -1;
>>>> +
>>>> +	br_dev = netdev_master_upper_dev_get_rcu((struct net_device *)ctx->dev);
>>>> +	if (!br_dev || !br_dev->netdev_ops->ndo_fill_forward_path)
>>>> +		return -1;
>>>> +
>>>> +	last_dev = ctx->dev;
>>>> +	path = dev_fwd_path(stack);
>>>> +	if (!path)
>>>> +		return -1;
>>>> +
>>>> +	memset(path, 0, sizeof(struct net_device_path));
>>>> +	ret = br_dev->netdev_ops->ndo_fill_forward_path(ctx, path);
>>>> +	if (ret < 0)
>>>> +		return -1;
>>>> +
>>>> +	if (!ctx->dev || WARN_ON_ONCE(last_dev == ctx->dev))
>>>> +		return -1;
> 
> * ^^^^^^^^^ here
> 
>>>> +
>>>> +	if (!netif_is_bridge_master(ctx->dev))
>>>
>>> hmm, do we expect ctx->dev to be a bridge master? Looking at
>>> br_fill_forward_path, it seems to be == fdb->dst->dev which
>>> should be the target port
>>
>> It would indeed be very unlikely. It was a left-over from code I wrote,
>> thinking that here I could handle cascaded bridges (via vlan-device). I
>> dropped that, since conntrack does not follow this flow.
>>
>> So would it be better to only make sure that ctx->dev is not a bridge
>> master?
>>
>> 	if (netif_is_bridge_master(ctx->dev))
>> 		return -1;
>>
>> 	return dev_fill_forward_path_common(ctx, stack);
>>
> 
> I think you misunderstood me, I don't think ctx->dev can ever be a bridge
> device because ctx->dev gets set to fdb->dst and fdbs that point to the bridge
> itself have fdb->dst == NULL but ctx->dev is checked against NULL earlier*
> so the bridge dev check doesn't make sense to me.

I see, thanks. I'll drop the check in v2.

>>>> +		return dev_fill_forward_path_common(ctx, stack);
>>>> +
>>>> +	path = dev_fwd_path(stack);
>>>> +	if (!path)
>>>> +		return -1;
>>>> +	path->type = DEV_PATH_ETHERNET;
>>>> +	path->dev = ctx->dev;
>>>>  
>>>>  	return ret;
>>>>  }
>>>> +EXPORT_SYMBOL_GPL(dev_fill_bridge_path);
>>>> +
>>>> +int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
>>>> +			  struct net_device_path_stack *stack)
>>>> +{
>>>> +	struct net_device_path_ctx ctx = {
>>>> +		.dev	= dev,
>>>> +	};
>>>> +
>>>> +	memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
>>>> +
>>>> +	stack->num_paths = 0;
>>>> +
>>>> +	return dev_fill_forward_path_common(&ctx, stack);
>>>> +}
>>>>  EXPORT_SYMBOL_GPL(dev_fill_forward_path);
>>>>  
>>>>  /**
>>>
>