| Message ID | 20211029015431.32516-1-xiangxia.m.yue@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | net: core: set skb useful vars in __bpf_tx_skb | expand |
On 10/29/21 3:54 AM, xiangxia.m.yue@gmail.com wrote: [...] > diff --git a/net/core/filter.c b/net/core/filter.c > index 4bace37a6a44..2dbff0944768 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -2107,9 +2107,19 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb) > return -ENETDOWN; > } > > - skb->dev = dev; > + /* The target netdevice (e.g. ifb) may use the: > + * - skb_iif > + * - redirected > + * - from_ingress > + */ > + skb->skb_iif = skb->dev->ifindex; This doesn't look right to me to set it unconditionally in tx path, isn't ifb_ri_tasklet() setting skb->skb_iif in this case (or __netif_receive_skb_core() in main rx path)? Also, I would suggest to add a proper BPF selftest which outlines the issue you're solving in here. > +#ifdef CONFIG_NET_CLS_ACT > + skb_set_redirected(skb, skb->tc_at_ingress); > +#else > skb->tstamp = 0; > +#endif > > + skb->dev = dev; > dev_xmit_recursion_inc(); > ret = dev_queue_xmit(skb); > dev_xmit_recursion_dec(); >
On Tue, Nov 2, 2021 at 5:46 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 10/29/21 3:54 AM, xiangxia.m.yue@gmail.com wrote: > [...] > > diff --git a/net/core/filter.c b/net/core/filter.c > > index 4bace37a6a44..2dbff0944768 100644 > > --- a/net/core/filter.c > > +++ b/net/core/filter.c > > @@ -2107,9 +2107,19 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb) > > return -ENETDOWN; > > } > > > > - skb->dev = dev; > > + /* The target netdevice (e.g. ifb) may use the: > > + * - skb_iif > > + * - redirected > > + * - from_ingress > > + */ > > + skb->skb_iif = skb->dev->ifindex; > > This doesn't look right to me to set it unconditionally in tx path, isn't ifb_ri_tasklet() > setting skb->skb_iif in this case (or __netif_receive_skb_core() in main rx path)? Hi the act_mirred set the skb->skb_iif, redirected and from_ingress. and __netif_receive_skb_core also set skb->skb_iif. so we can use the act_mirred to ifb in ingress or egress path. For ingress, when we use the bpf_redirct to ifb, we should set redirected, and from_ingress. For egress, when we use the bpf_redirct to ifb, we should skb_iif , set redirected, and from_ingress. > Also, I would suggest to add a proper BPF selftest which outlines the issue you're solving > in here. Ok, thanks. > > +#ifdef CONFIG_NET_CLS_ACT > > + skb_set_redirected(skb, skb->tc_at_ingress); > > +#else > > skb->tstamp = 0; > > +#endif > > > > + skb->dev = dev; > > dev_xmit_recursion_inc(); > > ret = dev_queue_xmit(skb); > > dev_xmit_recursion_dec(); > > >
On Tue, Nov 2, 2021 at 9:58 AM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Tue, Nov 2, 2021 at 5:46 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > > On 10/29/21 3:54 AM, xiangxia.m.yue@gmail.com wrote: > > [...] > > > diff --git a/net/core/filter.c b/net/core/filter.c > > > index 4bace37a6a44..2dbff0944768 100644 > > > --- a/net/core/filter.c > > > +++ b/net/core/filter.c > > > @@ -2107,9 +2107,19 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb) > > > return -ENETDOWN; > > > } > > > > > > - skb->dev = dev; > > > + /* The target netdevice (e.g. ifb) may use the: > > > + * - skb_iif > > > + * - redirected > > > + * - from_ingress > > > + */ > > > + skb->skb_iif = skb->dev->ifindex; > > > > This doesn't look right to me to set it unconditionally in tx path, isn't ifb_ri_tasklet() > > setting skb->skb_iif in this case (or __netif_receive_skb_core() in main rx path)? > Hi > the act_mirred set the skb->skb_iif, redirected and from_ingress. and > __netif_receive_skb_core also set skb->skb_iif. > so we can use the act_mirred to ifb in ingress or egress path. > For ingress, when we use the bpf_redirct to ifb, we should set > redirected, and from_ingress. > For egress, when we use the bpf_redirct to ifb, we should skb_iif , > set redirected, and from_ingress. Hi Daniel, because we don't know bpf_redirct invoked in tx path, or rx path. can we set the skb->skb_iif unconditionally in bpf_redirct? any thoughts? > > Also, I would suggest to add a proper BPF selftest which outlines the issue you're solving > > in here. > Ok, thanks. > > > +#ifdef CONFIG_NET_CLS_ACT > > > + skb_set_redirected(skb, skb->tc_at_ingress); > > > +#else > > > skb->tstamp = 0; > > > +#endif > > > > > > + skb->dev = dev; > > > dev_xmit_recursion_inc(); > > > ret = dev_queue_xmit(skb); > > > dev_xmit_recursion_dec(); > > > > > > > > -- > Best regards, Tonghao
diff --git a/net/core/filter.c b/net/core/filter.c index 4bace37a6a44..2dbff0944768 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -2107,9 +2107,19 @@ static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb) return -ENETDOWN; } - skb->dev = dev; + /* The target netdevice (e.g. ifb) may use the: + * - skb_iif + * - redirected + * - from_ingress + */ + skb->skb_iif = skb->dev->ifindex; +#ifdef CONFIG_NET_CLS_ACT + skb_set_redirected(skb, skb->tc_at_ingress); +#else skb->tstamp = 0; +#endif + skb->dev = dev; dev_xmit_recursion_inc(); ret = dev_queue_xmit(skb); dev_xmit_recursion_dec();