| Message ID | 20180507205723.28821-2-ecklm94@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [RFC,nft] WIP: Introducing socket matching | expand |
Máté Eckl <ecklm94@gmail.com> wrote: > I have been working on a skeleton for socket matching which is required by > tproxy support. > > See the WIP patch below and please comment if you have something to note. Excellent, thanks for working on this. > My thoughts: > * The parser is fine with this version of socket matching, matching the flags is > still to be implemented. > * We could treat this tproxy specifically all the way (eg. `tproxy socket`), but I > think this solution is more extensible and flexible. Matching flags with this > syntax makes other socket flags matchable and thus usable outside of the > use-case of transparent proxying. > * `isset` is probably not the best keyword to describe this, I have also thought > of `present` but maybe you also have some suggestions. If we want to match > socket flags this way, we need a keyword here. > > Regards, > Máté > > -- 8< -- > === Basic matching === > > eg.: `meta socket isset 1` > > This matches when there is a socket with the destination ip address > assigned to it as local address. I like this, but see below. > The new keyword `isset` represents a boolean, and it can later be reused > for the pointer type meta attributes, where the attribute is not > necessarily present at the time these rules are evaluated. For example > sk_user_data, sk_security, etc. What about re-using the 'exists' keyword for this? "meta socket exists"? Altough *I* would expect that this is eqivalent of skb->sk != NULL, not result of a lookup in the socket hash tables, so i would prefer something that makes this more obvious. > === Socket specific matching === > > `meta socket flags <flags>` > > This would match when `meta socket isset` matches AND the given flags > are set on the socket. Where is the 'isset' keyword here? meta socket exists flags ... won't work as above is really just a short-version of: meta socket eq 1 flags .. Which will match meta_expr relational_expr integer_expr flags), so we don't know what 'flags' is here, and where it refers to. Perhaps its better to use tproxy socket exists [ flags ], as that would allow to create a new statement where we know the context/structure and where the flags apply to. We can also re-purpose such a statement later to get TPROXY working: tproxy socket to 1.2.3.4:8080 flags ... Just some suggestions, I'm by no means good at defining ui things :-/ > --- a/src/meta.c > +++ b/src/meta.c > @@ -439,6 +439,8 @@ static const struct meta_template meta_templates[] = { > BYTEORDER_BIG_ENDIAN), /* avoid conversion; doesn't have endianess */ > [NFT_META_SECPATH] = META_TEMPLATE("secpath", &boolean_type, > BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), > + [NFT_META_SUBKEY_ISSET] = META_TEMPLATE("isset", &boolean_type, > + 1 , BYTEORDER_HOST_ENDIAN), The NFT_META_XXXXX define the elements that can be accessed, by the meta expression, such as skb->secpath or skb->mark. E.g. when user says 'meta mark 42' then meta expression is created with NFT_META_MARK key attribute, so kernel will know it has to fetch skb->mark. So, this would be something like [NFT_META_SK] = META_TEMPLATE("sk", &boolean_type, BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), I suggest to use BITS_PER_BYTE rather than 'bit' as the nft vm can access bytes but needs to generate shifts/masks to access bits. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the comments Florian. (Sorry I missed the mailing list form my first response.) Florian Westphal <fw@strlen.de> ezt írta (időpont: 2018. máj. 7., H, 23:23): > Máté Eckl <ecklm94@gmail.com> wrote: > > I have been working on a skeleton for socket matching which is required by > > tproxy support. > > > > See the WIP patch below and please comment if you have something to note. > Excellent, thanks for working on this. > > My thoughts: > > * The parser is fine with this version of socket matching, matching the flags is > > still to be implemented. > > * We could treat this tproxy specifically all the way (eg. `tproxy socket`), but I > > think this solution is more extensible and flexible. Matching flags with this > > syntax makes other socket flags matchable and thus usable outside of the > > use-case of transparent proxying. > > * `isset` is probably not the best keyword to describe this, I have also thought > > of `present` but maybe you also have some suggestions. If we want to match > > socket flags this way, we need a keyword here. > > > > Regards, > > Máté > > > > -- 8< -- > > === Basic matching === > > > > eg.: `meta socket isset 1` > > > > This matches when there is a socket with the destination ip address > > assigned to it as local address. > I like this, but see below. > > The new keyword `isset` represents a boolean, and it can later be reused > > for the pointer type meta attributes, where the attribute is not > > necessarily present at the time these rules are evaluated. For example > > sk_user_data, sk_security, etc. > What about re-using the 'exists' keyword for this? > "meta socket exists"? This is a good idea, I didn't notice this one. > Altough *I* would expect that this is eqivalent of > skb->sk != NULL, not result of a lookup in the socket hash tables, so i > would prefer something that makes this more obvious. Actually I think isset is more of what you say. That's why I didn't think it to be a good solution. Exists seems to be more global, so it does not have to be set, it is enough that it exists and can be found, so a lookup can be part of the decision. But I will think about this. > > === Socket specific matching === > > > > `meta socket flags <flags>` > > > > This would match when `meta socket isset` matches AND the given flags > > are set on the socket. > Where is the 'isset' keyword here? > meta socket exists flags ... > won't work as above is really just a short-version of: > meta socket eq 1 flags .. I thought of this as a totally independent keyword. In my plan the command line description would look like this: meta socket < exists <val> | flags <flags> > So I would not match flags with the `exists` keyword. This solves ambiguity, and it seemed to be intuitive for me. Matching flags on NULL is 0 or false anyways, so I didn't think, exists keyword is necessary here. The lookup should be made in both cases. > Which will match > meta_expr relational_expr integer_expr flags), so we don't know what > 'flags' is here, and where it refers to. > Perhaps its better to use > tproxy socket exists [ flags ], as that would allow > to create a new statement where we know the context/structure > and where the flags apply to. > We can also re-purpose such a statement later to get TPROXY working: > tproxy socket to 1.2.3.4:8080 flags ... > Just some suggestions, I'm by no means good at defining ui things :-/ My problem with this is that using the tproxy "target" does not require the socket matching to work in every case. Although it is true that those cases are rare but can possibly be useful for someone. So this syntax does not seem to be appropriate for socket matching, as it can be useful in other use-cases and is not *necessarily* part of a tproxy setup. So if you accept the description I gave on my plan above, I would prefer that. Does anyone have any other suggestion regarding this? > > --- a/src/meta.c > > +++ b/src/meta.c > > @@ -439,6 +439,8 @@ static const struct meta_template meta_templates[] = { > > BYTEORDER_BIG_ENDIAN), /* avoid conversion; doesn't have endianess */ > > [NFT_META_SECPATH] = META_TEMPLATE("secpath", &boolean_type, > > BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), > > + [NFT_META_SUBKEY_ISSET] = META_TEMPLATE("isset", &boolean_type, > > + 1 , BYTEORDER_HOST_ENDIAN), > The NFT_META_XXXXX define the elements that can be accessed, by the > meta expression, such as skb->secpath or skb->mark. > E.g. when user says 'meta mark 42' then meta expression is created with > NFT_META_MARK key attribute, so kernel will know it has to fetch > skb->mark. > So, this would be something like > [NFT_META_SK] = META_TEMPLATE("sk", &boolean_type, > BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), > I suggest to use BITS_PER_BYTE rather than 'bit' as the nft vm can > access bytes but needs to generate shifts/masks to access bits. Ok, I will consider this. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --- a/src/meta.c > > +++ b/src/meta.c > > @@ -439,6 +439,8 @@ static const struct meta_template meta_templates[] = { > > BYTEORDER_BIG_ENDIAN), /* avoid conversion; doesn't have endianess */ > > [NFT_META_SECPATH] = META_TEMPLATE("secpath", &boolean_type, > > BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), > > + [NFT_META_SUBKEY_ISSET] = META_TEMPLATE("isset", &boolean_type, > > + 1 , BYTEORDER_HOST_ENDIAN), > The NFT_META_XXXXX define the elements that can be accessed, by the > meta expression, such as skb->secpath or skb->mark. > E.g. when user says 'meta mark 42' then meta expression is created with > NFT_META_MARK key attribute, so kernel will know it has to fetch > skb->mark. > So, this would be something like > [NFT_META_SK] = META_TEMPLATE("sk", &boolean_type, > BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), Is it going to be something like one of the following? 1) if(skbuff->sk != NULL) OR 2) if(skbuff->sk->socket != NULL) And when matching flags, I guess I should examine `skbuff->sok->socket->ops`. I am a bit confused about the different socket structures right now. Until now I tought that `struct sock` stores the metadata of a packet. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eckl, Máté <ecklm94@gmail.com> wrote: > > > --- a/src/meta.c > > > +++ b/src/meta.c > > > @@ -439,6 +439,8 @@ static const struct meta_template meta_templates[] > = { > > > BYTEORDER_BIG_ENDIAN), /* > avoid conversion; doesn't have endianess */ > > > [NFT_META_SECPATH] = META_TEMPLATE("secpath", &boolean_type, > > > BITS_PER_BYTE, > BYTEORDER_HOST_ENDIAN), > > > + [NFT_META_SUBKEY_ISSET] = META_TEMPLATE("isset", &boolean_type, > > > + 1 , > BYTEORDER_HOST_ENDIAN), > > > The NFT_META_XXXXX define the elements that can be accessed, by the > > meta expression, such as skb->secpath or skb->mark. > > > E.g. when user says 'meta mark 42' then meta expression is created with > > NFT_META_MARK key attribute, so kernel will know it has to fetch > > skb->mark. > > > So, this would be something like > > > [NFT_META_SK] = META_TEMPLATE("sk", &boolean_type, > > BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), > > Is it going to be something like one of the following? > > 1) if(skbuff->sk != NULL) > OR > 2) if(skbuff->sk->socket != NULL) 1), then NFT_META_SK would fill destination register with 0 in case sk_buff->sk is NULL, and 1 otherwise. > And when matching flags, I guess I should examine > `skbuff->sok->socket->ops`. No need for that, see net/netfilter/xt_socket.c . But you don't even need to change kernel code, you could implement the userspace part and then pass xt_socket_mtinfo1 struct to the kernel. I will help you to do this once the parser parts are set up. > I am a bit confused about the different socket structures right now. Until > now I tought that `struct sock` stores the metadata of a packet. struct sk_buff stores the packet meta data (checksums, interface, header pointers, payload, routing information and the like). Incoming packets (before routing) have no socket (yet), in INPUT they might have one. Forwarded packets don't have a socket. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
=== Basic matching === eg.: `meta socket isset 1` This matches when there is a socket with the destination ip address assigned to it as local address. The new keyword `isset` represents a boolean, and it can later be reused for the pointer type meta attributes, where the attribute is not necessarily present at the time these rules are evaluated. For example sk_user_data, sk_security, etc. === Socket specific matching === `meta socket flags <flags>` This would match when `meta socket isset` matches AND the given flags are set on the socket. Signed-off-by: Máté Eckl <ecklm94@gmail.com> --- include/linux/netfilter/nf_tables.h | 2 ++ src/meta.c | 2 ++ src/parser_bison.y | 15 ++++++++++++++- src/scanner.l | 2 ++ 4 files changed, 20 insertions(+), 1 deletion(-) diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h index 517a39a..0719726 100644 --- a/include/linux/netfilter/nf_tables.h +++ b/include/linux/netfilter/nf_tables.h @@ -788,6 +788,7 @@ enum nft_exthdr_attributes { * @NFT_META_CGROUP: socket control group (skb->sk->sk_classid) * @NFT_META_PRANDOM: a 32bit pseudo-random number * @NFT_META_SECPATH: boolean, secpath_exists (!!skb->sp) + * @NFT_META_SUBKEY_ISSET: boolean, the subkey is set */ enum nft_meta_keys { NFT_META_LEN, @@ -816,6 +817,7 @@ enum nft_meta_keys { NFT_META_CGROUP, NFT_META_PRANDOM, NFT_META_SECPATH, + NFT_META_SUBKEY_ISSET, }; /** diff --git a/src/meta.c b/src/meta.c index 3012efa..7bbe4b1 100644 --- a/src/meta.c +++ b/src/meta.c @@ -439,6 +439,8 @@ static const struct meta_template meta_templates[] = { BYTEORDER_BIG_ENDIAN), /* avoid conversion; doesn't have endianess */ [NFT_META_SECPATH] = META_TEMPLATE("secpath", &boolean_type, BITS_PER_BYTE, BYTEORDER_HOST_ENDIAN), + [NFT_META_SUBKEY_ISSET] = META_TEMPLATE("isset", &boolean_type, + 1 , BYTEORDER_HOST_ENDIAN), }; static bool meta_key_is_qualified(enum nft_meta_keys key) diff --git a/src/parser_bison.y b/src/parser_bison.y index 7238a94..ecccd06 100644 --- a/src/parser_bison.y +++ b/src/parser_bison.y @@ -181,6 +181,7 @@ int nft_lex(void *, void *, void *); %token DASH "-" %token AT "@" %token VMAP "vmap" +%token ISSET "isset" %token INCLUDE "include" %token DEFINE "define" @@ -400,6 +401,7 @@ int nft_lex(void *, void *, void *); %token IIFGROUP "iifgroup" %token OIFGROUP "oifgroup" %token CGROUP "cgroup" +%token SOCKET "socket" %token CLASSID "classid" %token NEXTHOP "nexthop" @@ -689,7 +691,7 @@ int nft_lex(void *, void *, void *); %type <expr> meta_expr %destructor { expr_free($$); } meta_expr -%type <val> meta_key meta_key_qualified meta_key_unqualified numgen_type +%type <val> meta_key meta_key_qualified meta_key_unqualified meta_key_extended meta_subkey numgen_type %type <val> nf_key_proto @@ -3452,6 +3454,10 @@ meta_expr : META meta_key $$ = meta_expr_alloc(&@$, key); } + | META meta_key_extended + { + $$ = meta_expr_alloc(&@$, $2); + } ; meta_key : meta_key_qualified @@ -3486,6 +3492,13 @@ meta_key_unqualified : MARK { $$ = NFT_META_MARK; } | CGROUP { $$ = NFT_META_CGROUP; } ; +meta_key_extended : SOCKET meta_subkey { $$ = $2; } +/* | SOCKET FLAGS { $$ = NFT_META_SOCKET_FLAGS; } +*/ ; + +meta_subkey : ISSET { $$ = NFT_META_SUBKEY_ISSET; } + ; + meta_stmt : META meta_key SET stmt_expr { $$ = meta_stmt_alloc(&@$, $2, $4); diff --git a/src/scanner.l b/src/scanner.l index 70366d1..1fe2424 100644 --- a/src/scanner.l +++ b/src/scanner.l @@ -231,6 +231,7 @@ addrstring ({macaddr}|{ip4addr}|{ip6addr}) "$" { return '$'; } "=" { return '='; } "vmap" { return VMAP; } +"isset" { return ISSET; } "include" { return INCLUDE; } "define" { return DEFINE; } @@ -495,6 +496,7 @@ addrstring ({macaddr}|{ip4addr}|{ip6addr}) "iifgroup" { return IIFGROUP; } "oifgroup" { return OIFGROUP; } "cgroup" { return CGROUP; } +"socket" { return SOCKET; } "classid" { return CLASSID; } "nexthop" { return NEXTHOP; }