[PATCH net v1] ipvlan: Fix return value of ipvlan_queue_xmit()

[PATCH net v1] ipvlan: Fix return value of ipvlan_queue_xmit()

[Cambda Zhu]

The ipvlan_queue_xmit() should return NET_XMIT_XXX,
but ipvlan_xmit_mode_l2/l3() returns rx_handler_result_t or NET_RX_XXX
in some cases. The skb to forward could be treated as xmitted
successfully.

Fixes: 2ad7bf363841 ("ipvlan: Initial check-in of the IPVLAN driver.")
Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
---
v1:
- Add Fixes tag.
---
 drivers/net/ipvlan/ipvlan_core.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
index ab5133eb1d51..e45817caaee8 100644
--- a/drivers/net/ipvlan/ipvlan_core.c
+++ b/drivers/net/ipvlan/ipvlan_core.c
@@ -585,7 +585,8 @@ static int ipvlan_xmit_mode_l3(struct sk_buff *skb, struct net_device *dev)
 				consume_skb(skb);
 				return NET_XMIT_DROP;
 			}
-			return ipvlan_rcv_frame(addr, &skb, true);
+			ipvlan_rcv_frame(addr, &skb, true);
+			return NET_XMIT_SUCCESS;
 		}
 	}
 out:
@@ -611,7 +612,8 @@ static int ipvlan_xmit_mode_l2(struct sk_buff *skb, struct net_device *dev)
 					consume_skb(skb);
 					return NET_XMIT_DROP;
 				}
-				return ipvlan_rcv_frame(addr, &skb, true);
+				ipvlan_rcv_frame(addr, &skb, true);
+				return NET_XMIT_SUCCESS;
 			}
 		}
 		skb = skb_share_check(skb, GFP_ATOMIC);
@@ -623,7 +625,8 @@ static int ipvlan_xmit_mode_l2(struct sk_buff *skb, struct net_device *dev)
 		 * the skb for the main-dev. At the RX side we just return
 		 * RX_PASS for it to be processed further on the stack.
 		 */
-		return dev_forward_skb(ipvlan->phy_dev, skb);
+		dev_forward_skb(ipvlan->phy_dev, skb);
+		return NET_XMIT_SUCCESS;
 
 	} else if (is_multicast_ether_addr(eth->h_dest)) {
 		skb_reset_mac_header(skb);
-- 
2.16.6

Re: [PATCH net v1] ipvlan: Fix return value of ipvlan_queue_xmit()

[Cambda Zhu]

> On Jun 16, 2023, at 14:34, Cambda Zhu <cambda@linux.alibaba.com> wrote:
> 
> The ipvlan_queue_xmit() should return NET_XMIT_XXX,
> but ipvlan_xmit_mode_l2/l3() returns rx_handler_result_t or NET_RX_XXX
> in some cases. The skb to forward could be treated as xmitted
> successfully.
> 
> Fixes: 2ad7bf363841 ("ipvlan: Initial check-in of the IPVLAN driver.")
> Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
> ---
> v1:
> - Add Fixes tag.
> ---
> drivers/net/ipvlan/ipvlan_core.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ipvlan/ipvlan_core.c b/drivers/net/ipvlan/ipvlan_core.c
> index ab5133eb1d51..e45817caaee8 100644
> --- a/drivers/net/ipvlan/ipvlan_core.c
> +++ b/drivers/net/ipvlan/ipvlan_core.c
> @@ -585,7 +585,8 @@ static int ipvlan_xmit_mode_l3(struct sk_buff *skb, struct net_device *dev)
> consume_skb(skb);
> return NET_XMIT_DROP;
> }
> - return ipvlan_rcv_frame(addr, &skb, true);
> + ipvlan_rcv_frame(addr, &skb, true);
> + return NET_XMIT_SUCCESS;
> }
> }
> out:
> @@ -611,7 +612,8 @@ static int ipvlan_xmit_mode_l2(struct sk_buff *skb, struct net_device *dev)
> consume_skb(skb);
> return NET_XMIT_DROP;
> }
> - return ipvlan_rcv_frame(addr, &skb, true);
> + ipvlan_rcv_frame(addr, &skb, true);
> + return NET_XMIT_SUCCESS;
> }
> }
> skb = skb_share_check(skb, GFP_ATOMIC);
> @@ -623,7 +625,8 @@ static int ipvlan_xmit_mode_l2(struct sk_buff *skb, struct net_device *dev)
> * the skb for the main-dev. At the RX side we just return
> * RX_PASS for it to be processed further on the stack.
> */
> - return dev_forward_skb(ipvlan->phy_dev, skb);
> + dev_forward_skb(ipvlan->phy_dev, skb);
> + return NET_XMIT_SUCCESS;
> 
> } else if (is_multicast_ether_addr(eth->h_dest)) {
> skb_reset_mac_header(skb);
> -- 
> 2.16.6

Add CC Mahesh.

Re: [PATCH net v1] ipvlan: Fix return value of ipvlan_queue_xmit()

[Simon Horman]

On Fri, Jun 16, 2023 at 02:34:36PM +0800, Cambda Zhu wrote:
> The ipvlan_queue_xmit() should return NET_XMIT_XXX,
> but ipvlan_xmit_mode_l2/l3() returns rx_handler_result_t or NET_RX_XXX
> in some cases. The skb to forward could be treated as xmitted
> successfully.
> 
> Fixes: 2ad7bf363841 ("ipvlan: Initial check-in of the IPVLAN driver.")
> Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>

Hi Cambda Zhu,

ipvlan_rcv_frame can return two distinct values - RX_HANDLER_CONSUMED and
RX_HANDLER_ANOTHER. Is it correct to treat these both as NET_XMIT_SUCCESS
in the xmit path? If so, perhaps it would be useful to explain why
in the commit message.

Re: [PATCH net v1] ipvlan: Fix return value of ipvlan_queue_xmit()

[Cambda Zhu]

> On Jun 16, 2023, at 17:11, Simon Horman <simon.horman@corigine.com> wrote:
> 
> On Fri, Jun 16, 2023 at 02:34:36PM +0800, Cambda Zhu wrote:
>> The ipvlan_queue_xmit() should return NET_XMIT_XXX,
>> but ipvlan_xmit_mode_l2/l3() returns rx_handler_result_t or NET_RX_XXX
>> in some cases. The skb to forward could be treated as xmitted
>> successfully.
>> 
>> Fixes: 2ad7bf363841 ("ipvlan: Initial check-in of the IPVLAN driver.")
>> Signed-off-by: Cambda Zhu <cambda@linux.alibaba.com>
> 
> Hi Cambda Zhu,
> 
> ipvlan_rcv_frame can return two distinct values - RX_HANDLER_CONSUMED and
> RX_HANDLER_ANOTHER. Is it correct to treat these both as NET_XMIT_SUCCESS
> in the xmit path? If so, perhaps it would be useful to explain why
> in the commit message.

The ipvlan_rcv_frame() will only return RX_HANDLER_CONSUMED in
ipvlan_xmit_mode_l2/l3() for local is true. It's equal to NET_XMIT_SUCCESS.
The dev_forward_skb() can return NET_RX_SUCCESS and NET_RX_DROP, and
returning NET_RX_DROP(NET_XMIT_DROP) will increase both ipvlan and
ipvlan->phy_dev drops counter. I think the drops should belong to
the rcv side, and the xmit side should return NET_XMIT_SUCCESS even
if rcv failed. However, I'm not sure if my opinion is right.

Thanks,
Cambda

Re: [PATCH net v1] ipvlan: Fix return value of ipvlan_queue_xmit()

[Jakub Kicinski]

On Fri, 16 Jun 2023 17:40:29 +0800 Cambda Zhu wrote:
> > ipvlan_rcv_frame can return two distinct values - RX_HANDLER_CONSUMED and
> > RX_HANDLER_ANOTHER. Is it correct to treat these both as NET_XMIT_SUCCESS
> > in the xmit path? If so, perhaps it would be useful to explain why
> > in the commit message.  
> 
> The ipvlan_rcv_frame() will only return RX_HANDLER_CONSUMED in
> ipvlan_xmit_mode_l2/l3() for local is true. It's equal to NET_XMIT_SUCCESS.
> The dev_forward_skb() can return NET_RX_SUCCESS and NET_RX_DROP, and
> returning NET_RX_DROP(NET_XMIT_DROP) will increase both ipvlan and
> ipvlan->phy_dev drops counter. I think the drops should belong to
> the rcv side, and the xmit side should return NET_XMIT_SUCCESS even
> if rcv failed. However, I'm not sure if my opinion is right.

Please add the explanation to the commit msg and CC Mahesh on the v2.