Re: [PATCH nf-next] netfilter: nf_tables: Expose socket mark
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
> > ---
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf-next] netfilter: nf_tables: Expose socket mark
On Thu, Jul 12, 2018 at 12:52:16AM +0200, Máté Eckl wrote:
> Signed-off-by: Máté Eckl
> ---
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf-next] netfilter: nf_tables: Expose socket mark
On Thu, Jul 12, 2018 at 01:11:33PM +0200, Florian Westphal wrote:
> Máté Eckl wrote:
> > Signed-off-by: Máté Eckl
> > ---
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH nf-next] netfilter: nf_tables: Expose socket mark
Máté Eckl wrote:
> Signed-off-by: Máté Eckl
> ---
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
