On Sun, 28 Jan 2018 19:22:12 -0800 Cong Wang <xiyou.wangc...@gmail.com> wrote:
> On Fri, Jan 26, 2018 at 11:57 AM, Eyal Birger <eyal.bir...@gmail.com> > wrote: > > On Fri, Jan 26, 2018 at 8:50 PM, Pablo Neira Ayuso > > <pa...@netfilter.org> wrote: > >> Isn't there a way to reject the use of this from ->change()? ie. > >> from control plane configuration. > > > > I wasn't able to find a simple way of doing so: > > > > - AFAIU tc filters are detached from the qdiscs they operate on via > > tcf_block instances > > that may be shared by different qdiscs. I was not able to be sure > > that filters attached to ingress qdiscs via tcf_blocks at > > configuration time cannot be later be shared > > with non ingress qdiscs. Nor was I able to find another classifier > > making the ingress/egress > > distinction at configuration time. > > > > - ematches are not provided with 'ingress/egress' information at > > 'change()' invocation, though > > of course the infrastructure could be extended to provide this, > > given the distinction is available. > > > > In the past you can check tp->q, but now we support shared tc filter > block, so it is hard. I think your v1 is okay, which just silently > passes the match on egress side. Or maybe we can just add a pr_info() > unconditionally in em_ipt_change() saying only ingress is supported. Thanks! The motivation for allowing only ingress was to avoid skb modifications on egress as when running the match on egress, skb->data must point to the L3 header. Looking again at the calling flow e.g. from __dev_queue_xmit(), I don't see a case where skb may be shared. Similarly on ingress flow, sch_handle_ingress() modifies the skb, and tc actions perform skb modification without share checking. So as far as I can tell skb_pull() on the match is safe. Is there a different code path I should be looking for? If that is the case, perhaps the v1 approach supporting both directions including skb_pull() can be resubmitted without the pr_notice once net-next is open. Eyal.
