On 26 June 2019 19:33:48 EEST, nikolay@cumulusnetworks.com wrote:

On 26 June 2019 19:18:35 EEST, Eyal Birger [off-list ref]
wrote:

quoted

Hi Nik,

On Wed, 26 Jun 2019 18:56:14 +0300
Nikolay Aleksandrov [off-list ref] wrote:

quoted

For NFPROTO_UNSPEC xt_matches there's no way to restrict the

matching

quoted

quoted

to a specific family, in order to do so we record the user-specified
family and later enforce it while doing the match.

v2: adjust changes to missing patch, was patch 04 in v1

Signed-off-by: Nikolay Aleksandrov <redacted>
---
 net/sched/em_ipt.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

..snip..

quoted

@@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb,

struct tcf_ematch *em, const struct em_ipt_match *im = (const void
*)em->data; struct xt_action_param acpar = {};
 	struct net_device *indev = NULL;
-	u8 nfproto = im->match->family;
 	struct nf_hook_state state;
+	u8 nfproto = im->nfproto;

Maybe I'm missing something now - but it's not really clear to me now
why keeping im->nfproto would be useful:

If NFPROTO_UNSPEC was provided by userspace then the actual nfproto
used
will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified
from userspace then it will equal im->match->family.

Is there any case where the resulting nfproto would differ as a result
of this patch?

Otherwise the patchset looks excellent to me.

Thanks!
Eyal.

Hi,
It's needed to limit the match only to the user-specified family
for unspec xt matches. The problem is otherwise im->match->family
stays at nfproto_unspec regardless of the user choice.

Thanks for reviewing the set. 

Cheers,
 Nik

Hm, while that is true, thinking more about it - mixing the user proto and the real proto
could be problematic since we no longer enforce them to be equal, but we check
the network header len based on the packet only and we can end up checking v4
len and parsing it as nfproto v6. 

I'll spin v3 with unspec only and we can restrict it later if needed.