| Message ID | 20180711225214.5515-2-ecklm94@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next] netfilter: nf_tables: Expose socket mark | expand |
Máté Eckl <ecklm94@gmail.com> wrote: > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > --- > include/uapi/linux/netfilter/nf_tables.h | 4 +++- > net/netfilter/nft_socket.c | 11 ++++++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > index 89438e68dc03..f466860bcf75 100644 > --- a/include/uapi/linux/netfilter/nf_tables.h > +++ b/include/uapi/linux/netfilter/nf_tables.h > @@ -921,10 +921,12 @@ enum nft_socket_attributes { > /* > * enum nft_socket_keys - nf_tables socket expression keys > * > - * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option_ > + * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option > + * @NFT_SOCKET_MARK: Value of the socket mark > */ > enum nft_socket_keys { > NFT_SOCKET_TRANSPARENT, > + NFT_SOCKET_MARK, > __NFT_SOCKET_MAX > }; > #define NFT_SOCKET_MAX (__NFT_SOCKET_MAX - 1) > diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c > index 74e1b3bd6954..3f46b2013e26 100644 > --- a/net/netfilter/nft_socket.c > +++ b/net/netfilter/nft_socket.c > @@ -40,7 +40,7 @@ static void nft_socket_eval(const struct nft_expr *expr, > } > > if(!sk) { > - nft_reg_store8(dest, 0); > + *dest = 0; > return; > } > > @@ -51,6 +51,12 @@ static void nft_socket_eval(const struct nft_expr *expr, > case NFT_SOCKET_TRANSPARENT: > nft_reg_store8(dest, inet_sk_transparent(sk)); > break; > + case NFT_SOCKET_MARK: > + if (sk_fullsock(sk)) > + *dest = inet_request_mark(sk, skb); I wonder if it wouldn't be better to use sk->sk_mark directly. If user wants to fallback to skb->mark they could do socket mark 0 meta mark 42 rather than socket mark 42 ... matching when skb->mark is 42 and sk_mark is 0, it seems unexpected to me. Rest looks great. -- 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
On Thu, Jul 12, 2018 at 01:11:33PM +0200, Florian Westphal wrote: > Máté Eckl <ecklm94@gmail.com> wrote: > > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > > --- > > include/uapi/linux/netfilter/nf_tables.h | 4 +++- > > net/netfilter/nft_socket.c | 11 ++++++++++- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > > index 89438e68dc03..f466860bcf75 100644 > > --- a/include/uapi/linux/netfilter/nf_tables.h > > +++ b/include/uapi/linux/netfilter/nf_tables.h > > @@ -921,10 +921,12 @@ enum nft_socket_attributes { > > /* > > * enum nft_socket_keys - nf_tables socket expression keys > > * > > - * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option_ > > + * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option > > + * @NFT_SOCKET_MARK: Value of the socket mark > > */ > > enum nft_socket_keys { > > NFT_SOCKET_TRANSPARENT, > > + NFT_SOCKET_MARK, > > __NFT_SOCKET_MAX > > }; > > #define NFT_SOCKET_MAX (__NFT_SOCKET_MAX - 1) > > diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c > > index 74e1b3bd6954..3f46b2013e26 100644 > > --- a/net/netfilter/nft_socket.c > > +++ b/net/netfilter/nft_socket.c > > @@ -40,7 +40,7 @@ static void nft_socket_eval(const struct nft_expr *expr, > > } > > > > if(!sk) { > > - nft_reg_store8(dest, 0); > > + *dest = 0; > > return; > > } > > > > @@ -51,6 +51,12 @@ static void nft_socket_eval(const struct nft_expr *expr, > > case NFT_SOCKET_TRANSPARENT: > > nft_reg_store8(dest, inet_sk_transparent(sk)); > > break; > > + case NFT_SOCKET_MARK: > > + if (sk_fullsock(sk)) > > + *dest = inet_request_mark(sk, skb); > > I wonder if it wouldn't be better to use sk->sk_mark directly. > If user wants to fallback to skb->mark they could do > > socket mark 0 meta mark 42 > > rather than > > socket mark 42 > > ... matching when skb->mark is 42 and sk_mark is 0, it seems > unexpected to me. Yes that will be better. I'll wait some time in case other comments come up and then I resubmit with this modification. > Rest looks great. -- 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
On Thu, Jul 12, 2018 at 12:52:16AM +0200, Máté Eckl wrote: > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > --- > include/uapi/linux/netfilter/nf_tables.h | 4 +++- > net/netfilter/nft_socket.c | 11 ++++++++++- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > index 89438e68dc03..f466860bcf75 100644 > --- a/include/uapi/linux/netfilter/nf_tables.h > +++ b/include/uapi/linux/netfilter/nf_tables.h > @@ -921,10 +921,12 @@ enum nft_socket_attributes { > /* > * enum nft_socket_keys - nf_tables socket expression keys > * > - * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option_ > + * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option > + * @NFT_SOCKET_MARK: Value of the socket mark > */ > enum nft_socket_keys { > NFT_SOCKET_TRANSPARENT, > + NFT_SOCKET_MARK, > __NFT_SOCKET_MAX > }; > #define NFT_SOCKET_MAX (__NFT_SOCKET_MAX - 1) > diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c > index 74e1b3bd6954..3f46b2013e26 100644 > --- a/net/netfilter/nft_socket.c > +++ b/net/netfilter/nft_socket.c > @@ -40,7 +40,7 @@ static void nft_socket_eval(const struct nft_expr *expr, > } > > if(!sk) { > - nft_reg_store8(dest, 0); > + *dest = 0; > return; I think this should be: if (!sk) regs->verdict.code = NFT_BREAK; So we make sure we skip further evaluation, because zero may be a valid mark. or better: if (!sk) goto out: ... out: regs->verdict.code = NFT_BREAK; so you consolidate this evaluation break path. An initial patch to fix what we have would be good to have. > } > > @@ -51,6 +51,12 @@ static void nft_socket_eval(const struct nft_expr *expr, > case NFT_SOCKET_TRANSPARENT: > nft_reg_store8(dest, inet_sk_transparent(sk)); > break; > + case NFT_SOCKET_MARK: > + if (sk_fullsock(sk)) > + *dest = inet_request_mark(sk, skb); > + else > + *dest = 0; Again, better break evaluation here, so I would do: if (!sk_fullsock(sk)) goto out; ... out: regs->verdict.code = NFT_BREAK; Thanks. -- 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
On Thu, Jul 12, 2018 at 04:26:54PM +0200, Pablo Neira Ayuso wrote: > On Thu, Jul 12, 2018 at 12:52:16AM +0200, Máté Eckl wrote: > > Signed-off-by: Máté Eckl <ecklm94@gmail.com> > > --- > > include/uapi/linux/netfilter/nf_tables.h | 4 +++- > > net/netfilter/nft_socket.c | 11 ++++++++++- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h > > index 89438e68dc03..f466860bcf75 100644 > > --- a/include/uapi/linux/netfilter/nf_tables.h > > +++ b/include/uapi/linux/netfilter/nf_tables.h > > @@ -921,10 +921,12 @@ enum nft_socket_attributes { > > /* > > * enum nft_socket_keys - nf_tables socket expression keys > > * > > - * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option_ > > + * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option > > + * @NFT_SOCKET_MARK: Value of the socket mark > > */ > > enum nft_socket_keys { > > NFT_SOCKET_TRANSPARENT, > > + NFT_SOCKET_MARK, > > __NFT_SOCKET_MAX > > }; > > #define NFT_SOCKET_MAX (__NFT_SOCKET_MAX - 1) > > diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c > > index 74e1b3bd6954..3f46b2013e26 100644 > > --- a/net/netfilter/nft_socket.c > > +++ b/net/netfilter/nft_socket.c > > @@ -40,7 +40,7 @@ static void nft_socket_eval(const struct nft_expr *expr, > > } > > > > if(!sk) { > > - nft_reg_store8(dest, 0); > > + *dest = 0; > > return; > > I think this should be: > > if (!sk) > regs->verdict.code = NFT_BREAK; > > So we make sure we skip further evaluation, because zero may be a > valid mark. > > or better: > > if (!sk) > goto out: > ... > out: > regs->verdict.code = NFT_BREAK; > > so you consolidate this evaluation break path. > > An initial patch to fix what we have would be good to have. > > > } > > > > @@ -51,6 +51,12 @@ static void nft_socket_eval(const struct nft_expr *expr, > > case NFT_SOCKET_TRANSPARENT: > > nft_reg_store8(dest, inet_sk_transparent(sk)); > > break; > > + case NFT_SOCKET_MARK: > > + if (sk_fullsock(sk)) > > + *dest = inet_request_mark(sk, skb); > > + else > > + *dest = 0; > > Again, better break evaluation here, so I would do: > > if (!sk_fullsock(sk)) > goto out; > > ... > out: > regs->verdict.code = NFT_BREAK; > > Thanks. Thanks for the observation, you are right, it should break evaluation. I'll fix this too. -- 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
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h index 89438e68dc03..f466860bcf75 100644 --- a/include/uapi/linux/netfilter/nf_tables.h +++ b/include/uapi/linux/netfilter/nf_tables.h @@ -921,10 +921,12 @@ enum nft_socket_attributes { /* * enum nft_socket_keys - nf_tables socket expression keys * - * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option_ + * @NFT_SOCKET_TRANSPARENT: Value of the IP(V6)_TRANSPARENT socket option + * @NFT_SOCKET_MARK: Value of the socket mark */ enum nft_socket_keys { NFT_SOCKET_TRANSPARENT, + NFT_SOCKET_MARK, __NFT_SOCKET_MAX }; #define NFT_SOCKET_MAX (__NFT_SOCKET_MAX - 1) diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c index 74e1b3bd6954..3f46b2013e26 100644 --- a/net/netfilter/nft_socket.c +++ b/net/netfilter/nft_socket.c @@ -40,7 +40,7 @@ static void nft_socket_eval(const struct nft_expr *expr, } if(!sk) { - nft_reg_store8(dest, 0); + *dest = 0; return; } @@ -51,6 +51,12 @@ static void nft_socket_eval(const struct nft_expr *expr, case NFT_SOCKET_TRANSPARENT: nft_reg_store8(dest, inet_sk_transparent(sk)); break; + case NFT_SOCKET_MARK: + if (sk_fullsock(sk)) + *dest = inet_request_mark(sk, skb); + else + *dest = 0; + break; default: WARN_ON(1); regs->verdict.code = NFT_BREAK; @@ -88,6 +94,9 @@ static int nft_socket_init(const struct nft_ctx *ctx, case NFT_SOCKET_TRANSPARENT: len = sizeof(u8); break; + case NFT_SOCKET_MARK: + len = sizeof(u32); + break; default: return -EOPNOTSUPP; }
Signed-off-by: Máté Eckl <ecklm94@gmail.com> --- include/uapi/linux/netfilter/nf_tables.h | 4 +++- net/netfilter/nft_socket.c | 11 ++++++++++- 2 files changed, 13 insertions(+), 2 deletions(-)