[nf-next] netfilter: nf_tables: Expose socket mark

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
Related show

Commit Message

Máté Eckl July 11, 2018, 10:52 p.m.
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(-)

Comments

Florian Westphal July 12, 2018, 11:11 a.m. | #1
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
Máté Eckl July 12, 2018, 12:38 p.m. | #2
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
Pablo Neira Ayuso July 12, 2018, 2:26 p.m. | #3
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
Máté Eckl July 12, 2018, 3:15 p.m. | #4
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

Patch

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;
 	}