| Message ID | 20230616063436.28760-1-cambda@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] ipvlan: Fix return value of ipvlan_queue_xmit() | expand |
> 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.
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.
> 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
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.
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);
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(-)