Le 08/12/2020 à 15:00, Phil Sutter a écrit : > Hi Nicolas, > > On Tue, Dec 08, 2020 at 10:02:16AM +0100, Nicolas Dichtel wrote: >> Le 07/12/2020 à 14:43, Phil Sutter a écrit : > [...] >>> diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c >>> index aa4cdcf69d471..24af61c95b4d4 100644 >>> --- a/net/xfrm/xfrm_interface.c >>> +++ b/net/xfrm/xfrm_interface.c >>> @@ -317,7 +317,8 @@ xfrmi_xmit2(struct sk_buff *skb, struct net_device *dev, struct flowi *fl) >>> skb_dst_set(skb, dst); >>> skb->dev = tdev; >>> >>> - err = dst_output(xi->net, skb->sk, skb); >>> + err = NF_HOOK(skb_dst(skb)->ops->family, NF_INET_LOCAL_OUT, xi->net, >> skb->protocol must be correctly set, maybe better to use it instead of >> skb_dst(skb)->ops->family? > > skb->protocol holds ETH_P_* values in network byte order, NF_HOOK() > expects an NFPROTO_* value, so this would at least not be a simple Yes, right. I forgot that. > >>> + skb->sk, skb, NULL, skb_dst(skb)->dev, dst_output); >> And here, tdev instead of skb_dst(skb)->dev ? > > Well yes, tdev was set to dst->dev earlier. Likewise I could use dst > directly instead of skb_dst(skb) to simplify the call a bit further. > OTOH I like how in the version above it is clear that skb's dst should > be used, irrespective of the code above (and any later changes that may > receive). No strong opinion though, so if your version is regarded the > preferred one, I'm fine with that. I would vote for tdev, because: - the reader don't have to wonder why tdev is used for dst->dev and not for NF_HOOK(); - tdev has probably been declared so that dst->dev is dereferenced only once; - using the same variable everywhere in the function eases code reading. Thank you, Nicolas
