[PATCH nf-next] netfilter: xt_socket: add XT_SOCKET_MATCHSOCKMARK flag and mark fields

[PATCH nf-next] netfilter: xt_socket: add XT_SOCKET_MATCHSOCKMARK flag and mark fields

[Harout Hedeshian]

xt_socket is useful for matching sockets with IP_TRANSPARENT and
taking some action on the matching packets. However, it lacks the
ability to match only a small subset of transparent sockets.

Suppose there are 2 applications, each with its own set of transparent
sockets. The first application wants all matching packets dropped,
while the second application wants them forwarded somewhere else.

Add the ability to match sockets based on the socket mark.

Now the 2 hypothetical applications can differentiate their sockets
based on a mark value set with SO_MARK.

iptables -t mangle -I PREROUTING -m socket --transparent --mark 10 -J act1
iptables -t mangle -I PREROUTING -m socket --transparent --mark 11 -J act2

The mark field also takes an optional mask and is invertable.

Signed-off-by: Harout Hedeshian <harouth@codeaurora.org>
---
 include/uapi/linux/netfilter/xt_socket.h |  10 +++
 net/netfilter/xt_socket.c                | 108 +++++++++++++++++++++++++++++--
 2 files changed, 112 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/netfilter/xt_socket.h b/include/uapi/linux/netfilter/xt_socket.h
index 6315e2a..3a7304e 100644
--- a/include/uapi/linux/netfilter/xt_socket.h
+++ b/include/uapi/linux/netfilter/xt_socket.h
@@ -6,6 +6,7 @@
 enum {
 	XT_SOCKET_TRANSPARENT = 1 << 0,
 	XT_SOCKET_NOWILDCARD = 1 << 1,
+	XT_SOCKET_MATCHSOCKMARK = 1 << 2,
 };
 
 struct xt_socket_mtinfo1 {
@@ -18,4 +19,13 @@ struct xt_socket_mtinfo2 {
 };
 #define XT_SOCKET_FLAGS_V2 (XT_SOCKET_TRANSPARENT | XT_SOCKET_NOWILDCARD)
 
+struct xt_socket_mtinfo3 {
+	__u8 flags;
+	__u32 mark, mask;
+	__u8 invert_mark;
+};
+#define XT_SOCKET_FLAGS_V3 (XT_SOCKET_TRANSPARENT \
+			   | XT_SOCKET_NOWILDCARD \
+			   | XT_SOCKET_MATCHSOCKMARK)
+
 #endif /* _XT_SOCKET_H */
diff --git a/net/netfilter/xt_socket.c b/net/netfilter/xt_socket.c
index e092cb0..abbba03 100644
--- a/net/netfilter/xt_socket.c
+++ b/net/netfilter/xt_socket.c
@@ -203,7 +203,7 @@ static struct sock *xt_socket_lookup_slow_v4(const struct sk_buff *skb,
 
 static bool
 socket_match(const struct sk_buff *skb, struct xt_action_param *par,
-	     const struct xt_socket_mtinfo1 *info)
+	     const struct xt_socket_mtinfo3 *info)
 {
 	struct sock *sk = skb->sk;
 
@@ -212,6 +212,7 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
 	if (sk) {
 		bool wildcard;
 		bool transparent = true;
+		bool markmatch = true;
 
 		/* Ignore sockets listening on INADDR_ANY,
 		 * unless XT_SOCKET_NOWILDCARD is set
@@ -226,10 +227,17 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
 		if (info->flags & XT_SOCKET_TRANSPARENT)
 			transparent = xt_socket_sk_is_transparent(sk);
 
+		/* Ignore sockets which are not marked a certain value
+		 * if XT_SOCKET_MATCHSOCKMARK is set
+		 */
+		if (info->flags & XT_SOCKET_MATCHSOCKMARK)
+			markmatch = ((sk->sk_mark & info->mask) == info->mark)
+				    ^ info->invert_mark;
+
 		if (sk != skb->sk)
 			sock_gen_put(sk);
 
-		if (wildcard || !transparent)
+		if (wildcard || !transparent || !markmatch)
 			sk = NULL;
 	}
 
@@ -239,8 +247,11 @@ socket_match(const struct sk_buff *skb, struct xt_action_param *par,
 static bool
 socket_mt4_v0(const struct sk_buff *skb, struct xt_action_param *par)
 {
-	static struct xt_socket_mtinfo1 xt_info_v0 = {
+	static struct xt_socket_mtinfo3 xt_info_v0 = {
 		.flags = 0,
+		.mark = 0,
+		.mask = 0,
+		.invert_mark = 0,
 	};
 
 	return socket_match(skb, par, &xt_info_v0);
@@ -249,6 +260,22 @@ socket_mt4_v0(const struct sk_buff *skb, struct xt_action_param *par)
 static bool
 socket_mt4_v1_v2(const struct sk_buff *skb, struct xt_action_param *par)
 {
+	struct xt_socket_mtinfo1 *xt_info  =
+				     (struct xt_socket_mtinfo1 *)par->matchinfo;
+	static struct xt_socket_mtinfo3 xt_info_v1 = {
+		.flags = 0,
+		.mark = 0,
+		.mask = 0,
+		.invert_mark = 0,
+	};
+	xt_info_v1.flags = xt_info->flags;
+
+	return socket_match(skb, par, &xt_info_v1);
+}
+
+static bool
+socket_mt4_v3(const struct sk_buff *skb, struct xt_action_param *par)
+{
 	return socket_match(skb, par, par->matchinfo);
 }
 
@@ -371,9 +398,10 @@ static struct sock *xt_socket_lookup_slow_v6(const struct sk_buff *skb,
 }
 
 static bool
-socket_mt6_v1_v2(const struct sk_buff *skb, struct xt_action_param *par)
+socket_match6(const struct sk_buff *skb,
+	      struct xt_action_param *par,
+	      const struct xt_socket_mtinfo3 *info)
 {
-	const struct xt_socket_mtinfo1 *info = (struct xt_socket_mtinfo1 *) par->matchinfo;
 	struct sock *sk = skb->sk;
 
 	if (!sk)
@@ -381,6 +409,7 @@ socket_mt6_v1_v2(const struct sk_buff *skb, struct xt_action_param *par)
 	if (sk) {
 		bool wildcard;
 		bool transparent = true;
+		bool markmatch = true;
 
 		/* Ignore sockets listening on INADDR_ANY
 		 * unless XT_SOCKET_NOWILDCARD is set
@@ -395,15 +424,44 @@ socket_mt6_v1_v2(const struct sk_buff *skb, struct xt_action_param *par)
 		if (info->flags & XT_SOCKET_TRANSPARENT)
 			transparent = xt_socket_sk_is_transparent(sk);
 
+		/* Ignore sockets which are not marked a certain value
+		 *  if XT_SOCKET_MATCHSOCKMARK is set
+		 */
+		if (info->flags & XT_SOCKET_MATCHSOCKMARK)
+			markmatch = ((sk->sk_mark & info->mask) == info->mark)
+				    ^ info->invert_mark;
+
 		if (sk != skb->sk)
 			sock_gen_put(sk);
 
-		if (wildcard || !transparent)
+		if (wildcard || !transparent || !markmatch)
 			sk = NULL;
 	}
 
 	return sk != NULL;
 }
+
+static bool
+socket_mt6_v1_v2(const struct sk_buff *skb, struct xt_action_param *par)
+{
+	struct xt_socket_mtinfo1 *xt_info  =
+				     (struct xt_socket_mtinfo1 *)par->matchinfo;
+	static struct xt_socket_mtinfo3 xt_info_v1 = {
+		.flags = 0,
+		.mark = 0,
+		.mask = 0,
+		.invert_mark = 0,
+	};
+	xt_info_v1.flags = xt_info->flags;
+
+	return socket_match6(skb, par, &xt_info_v1);
+}
+
+static bool
+socket_mt6_v3(const struct sk_buff *skb, struct xt_action_param *par)
+{
+	return socket_match6(skb, par, par->matchinfo);
+}
 #endif
 
 static int socket_mt_v1_check(const struct xt_mtchk_param *par)
@@ -428,6 +486,20 @@ static int socket_mt_v2_check(const struct xt_mtchk_param *par)
 	return 0;
 }
 
+static int socket_mt_v3_check(const struct xt_mtchk_param *par)
+{
+	const struct xt_socket_mtinfo3 *info =
+				    (struct xt_socket_mtinfo3 *)par->matchinfo;
+
+	if (info->flags & ~XT_SOCKET_FLAGS_V3) {
+		pr_info("unknown flags 0x%x\n",
+			info->flags & ~XT_SOCKET_FLAGS_V3);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static struct xt_match socket_mt_reg[] __read_mostly = {
 	{
 		.name		= "socket",
@@ -486,6 +558,30 @@ static struct xt_match socket_mt_reg[] __read_mostly = {
 		.me		= THIS_MODULE,
 	},
 #endif
+	{
+		.name		= "socket",
+		.revision	= 3,
+		.family		= NFPROTO_IPV4,
+		.match		= socket_mt4_v3,
+		.checkentry	= socket_mt_v3_check,
+		.matchsize	= sizeof(struct xt_socket_mtinfo3),
+		.hooks		= (1 << NF_INET_PRE_ROUTING) |
+				  (1 << NF_INET_LOCAL_IN),
+		.me		= THIS_MODULE,
+	},
+#ifdef XT_SOCKET_HAVE_IPV6
+	{
+		.name		= "socket",
+		.revision	= 3,
+		.family		= NFPROTO_IPV6,
+		.match		= socket_mt6_v3,
+		.checkentry	= socket_mt_v3_check,
+		.matchsize	= sizeof(struct xt_socket_mtinfo3),
+		.hooks		= (1 << NF_INET_PRE_ROUTING) |
+				  (1 << NF_INET_LOCAL_IN),
+		.me		= THIS_MODULE,
+	},
+#endif
 };
 
 static int __init socket_mt_init(void)
-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH nf-next] netfilter: xt_socket: add XT_SOCKET_MATCHSOCKMARK flag and mark fields

[Pablo Neira Ayuso]

On Fri, Jun 12, 2015 at 04:39:55PM -0600, Harout Hedeshian wrote:
> xt_socket is useful for matching sockets with IP_TRANSPARENT and
> taking some action on the matching packets. However, it lacks the
> ability to match only a small subset of transparent sockets.
>
> Suppose there are 2 applications, each with its own set of transparent
> sockets. The first application wants all matching packets dropped,
> while the second application wants them forwarded somewhere else.
>
> Add the ability to match sockets based on the socket mark.
>
> Now the 2 hypothetical applications can differentiate their sockets
> based on a mark value set with SO_MARK.
>
> iptables -t mangle -I PREROUTING -m socket --transparent --mark 10 -J act1
> iptables -t mangle -I PREROUTING -m socket --transparent --mark 11 -J act2

Wouldn't it be better to restore the sk_mark to skb->mark? I mean:

        iptables -t mangle -I PREROUTING \
                -m socket --transparent --restore-skmark -j myskchain

Thus, you can use -m mark from the 'myskchain' without having to fetch
and inspect the sk over and over again as it happens with this
extension.

RE: [PATCH nf-next] netfilter: xt_socket: add XT_SOCKET_MATCHSOCKMARK flag and mark fields

[Harout Hedeshian]

> -----Original Message-----
> From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> Sent: Monday, June 15, 2015 10:23 AM
> To: Harout Hedeshian
> Cc: netfilter-devel@vger.kernel.org; Lorenzo Colitti
> Subject: Re: [PATCH nf-next] netfilter: xt_socket: add
> XT_SOCKET_MATCHSOCKMARK flag and mark fields
> 
> On Fri, Jun 12, 2015 at 04:39:55PM -0600, Harout Hedeshian wrote:
> > xt_socket is useful for matching sockets with IP_TRANSPARENT and
> > taking some action on the matching packets. However, it lacks the
> > ability to match only a small subset of transparent sockets.
> >
> > Suppose there are 2 applications, each with its own set of transparent
> > sockets. The first application wants all matching packets dropped,
> > while the second application wants them forwarded somewhere else.
> >
> > Add the ability to match sockets based on the socket mark.
> >
> > Now the 2 hypothetical applications can differentiate their sockets
> > based on a mark value set with SO_MARK.
> >
> > iptables -t mangle -I PREROUTING -m socket --transparent --mark 10 -J
> > act1 iptables -t mangle -I PREROUTING -m socket --transparent --mark
> > 11 -J act2
> 
> Wouldn't it be better to restore the sk_mark to skb->mark? I mean:
> 
>         iptables -t mangle -I PREROUTING \
>                 -m socket --transparent --restore-skmark -j myskchain
> 
> Thus, you can use -m mark from the 'myskchain' without having to fetch
> and inspect the sk over and over again as it happens with this
> extension.

Yes, I think that can work. Let me try that out and I'll submit a new patch.

RE: [PATCH nf-next] netfilter: xt_socket: add XT_SOCKET_MATCHSOCKMARK flag and mark fields

[Harout Hedeshian]

> -----Original Message-----
> From: Harout Hedeshian [mailto:harouth@codeaurora.org]
> Sent: Monday, June 15, 2015 11:42 AM
> To: 'Pablo Neira Ayuso'
> Cc: 'netfilter-devel@vger.kernel.org'; 'Lorenzo Colitti'
> Subject: RE: [PATCH nf-next] netfilter: xt_socket: add
> XT_SOCKET_MATCHSOCKMARK flag and mark fields
> 
> 
> 
> > -----Original Message-----
> > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > Sent: Monday, June 15, 2015 10:23 AM
> > To: Harout Hedeshian
> > Cc: netfilter-devel@vger.kernel.org; Lorenzo Colitti
> > Subject: Re: [PATCH nf-next] netfilter: xt_socket: add
> > XT_SOCKET_MATCHSOCKMARK flag and mark fields
> >
> > On Fri, Jun 12, 2015 at 04:39:55PM -0600, Harout Hedeshian wrote:
> > > xt_socket is useful for matching sockets with IP_TRANSPARENT and
> > > taking some action on the matching packets. However, it lacks the
> > > ability to match only a small subset of transparent sockets.
> > >
> > > Suppose there are 2 applications, each with its own set of
> > > transparent sockets. The first application wants all matching
> > > packets dropped, while the second application wants them forwarded
> somewhere else.
> > >
> > > Add the ability to match sockets based on the socket mark.
> > >
> > > Now the 2 hypothetical applications can differentiate their sockets
> > > based on a mark value set with SO_MARK.
> > >
> > > iptables -t mangle -I PREROUTING -m socket --transparent --mark 10
> > > -J
> > > act1 iptables -t mangle -I PREROUTING -m socket --transparent --mark
> > > 11 -J act2
> >
> > Wouldn't it be better to restore the sk_mark to skb->mark? I mean:
> >
> >         iptables -t mangle -I PREROUTING \
> >                 -m socket --transparent --restore-skmark -j myskchain
> >	
> > Thus, you can use -m mark from the 'myskchain' without having to fetch
> > and inspect the sk over and over again as it happens with this
> > extension.
> 
> Yes, I think that can work. Let me try that out and I'll submit a new
> patch.

Actually, on second thought, I'm not so sure. This socket lookup is
happening as part of a match operation in xt_socket.c.
>From x_tables.h, I can see that match functions are not supposed to modify
the skb:

struct xt_match{
...
bool (*match)(const struct sk_buff *skb, struct xt_action_param *);


In that case, would this even be valid?
-m socket --transparent --restore-skmark

Keeping in mind --restore-skmark is happening as part of -m socket

I would think we would need a whole new target to handle something like
this:
iptables -t mangle -I PREROUTING -m socket --transparent --no-wildcard -j
SOCKET --restore-skmark

Since the target invocation would be separate, we would need a second socket
lookup? Seems perhaps a little overkill...

Harout

Re: [PATCH nf-next] netfilter: xt_socket: add XT_SOCKET_MATCHSOCKMARK flag and mark fields

[Pablo Neira Ayuso]

On Mon, Jun 15, 2015 at 01:39:03PM -0600, Harout Hedeshian wrote:
> 
> 
> > -----Original Message-----
> > From: Harout Hedeshian [mailto:harouth@codeaurora.org]
> > Sent: Monday, June 15, 2015 11:42 AM
> > To: 'Pablo Neira Ayuso'
> > Cc: 'netfilter-devel@vger.kernel.org'; 'Lorenzo Colitti'
> > Subject: RE: [PATCH nf-next] netfilter: xt_socket: add
> > XT_SOCKET_MATCHSOCKMARK flag and mark fields
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> > > Sent: Monday, June 15, 2015 10:23 AM
> > > To: Harout Hedeshian
> > > Cc: netfilter-devel@vger.kernel.org; Lorenzo Colitti
> > > Subject: Re: [PATCH nf-next] netfilter: xt_socket: add
> > > XT_SOCKET_MATCHSOCKMARK flag and mark fields
> > >
> > > On Fri, Jun 12, 2015 at 04:39:55PM -0600, Harout Hedeshian wrote:
> > > > xt_socket is useful for matching sockets with IP_TRANSPARENT and
> > > > taking some action on the matching packets. However, it lacks the
> > > > ability to match only a small subset of transparent sockets.
> > > >
> > > > Suppose there are 2 applications, each with its own set of
> > > > transparent sockets. The first application wants all matching
> > > > packets dropped, while the second application wants them forwarded
> > somewhere else.
> > > >
> > > > Add the ability to match sockets based on the socket mark.
> > > >
> > > > Now the 2 hypothetical applications can differentiate their sockets
> > > > based on a mark value set with SO_MARK.
> > > >
> > > > iptables -t mangle -I PREROUTING -m socket --transparent --mark 10
> > > > -J
> > > > act1 iptables -t mangle -I PREROUTING -m socket --transparent --mark
> > > > 11 -J act2
> > >
> > > Wouldn't it be better to restore the sk_mark to skb->mark? I mean:
> > >
> > >         iptables -t mangle -I PREROUTING \
> > >                 -m socket --transparent --restore-skmark -j myskchain
> > >	
> > > Thus, you can use -m mark from the 'myskchain' without having to fetch
> > > and inspect the sk over and over again as it happens with this
> > > extension.
> > 
> > Yes, I think that can work. Let me try that out and I'll submit a new
> > patch.
> 
> Actually, on second thought, I'm not so sure. This socket lookup is
> happening as part of a match operation in xt_socket.c.
> From x_tables.h, I can see that match functions are not supposed to modify
> the skb:
> 
> struct xt_match{
> ...
> bool (*match)(const struct sk_buff *skb, struct xt_action_param *);
>
> In that case, would this even be valid?
> -m socket --transparent --restore-skmark
> 
> Keeping in mind --restore-skmark is happening as part of -m socket
> 
> I would think we would need a whole new target to handle something like
> this:
> iptables -t mangle -I PREROUTING -m socket --transparent --no-wildcard -j
> SOCKET --restore-skmark
> 
> Since the target invocation would be separate, we would need a second socket
> lookup? Seems perhaps a little overkill...

Yes, it's simply overkill to add a new target to add this.

So just cast it:

        struct sk_buff *pskb = (struct sk_buff *)skb;

iptables matches were originally design not to modify anything, but it
can lead us to situations like this. That's one of the reasons why we
have no distinction between matches and targets in nftables anymore.

Thanks.

RE: [PATCH nf-next] netfilter: xt_socket: add XT_SOCKET_MATCHSOCKMARK flag and mark fields

[Harout Hedeshian]

> -----Original Message-----
> From: Pablo Neira Ayuso [mailto:pablo@netfilter.org]
> Sent: Monday, June 15, 2015 2:58 PM
> To: Harout Hedeshian
> Cc: netfilter-devel@vger.kernel.org; 'Lorenzo Colitti'
> Subject: Re: [PATCH nf-next] netfilter: xt_socket: add
> XT_SOCKET_MATCHSOCKMARK flag and mark fields
> 
> >
> > Actually, on second thought, I'm not so sure. This socket lookup is
> > happening as part of a match operation in xt_socket.c.
> > From x_tables.h, I can see that match functions are not supposed to
> > modify the skb:
> >
> > struct xt_match{
> > ...
> > bool (*match)(const struct sk_buff *skb, struct xt_action_param *);
> >
> > In that case, would this even be valid?
> > -m socket --transparent --restore-skmark
> >
> > Keeping in mind --restore-skmark is happening as part of -m socket
> >
> > I would think we would need a whole new target to handle something
> > like
> > this:
> > iptables -t mangle -I PREROUTING -m socket --transparent --no-wildcard
> > -j SOCKET --restore-skmark
> >
> > Since the target invocation would be separate, we would need a second
> > socket lookup? Seems perhaps a little overkill...
> 
> Yes, it's simply overkill to add a new target to add this.
> 
> So just cast it:
Yuck...

>         struct sk_buff *pskb = (struct sk_buff *)skb;
> 
> iptables matches were originally design not to modify anything, but it
> can lead us to situations like this. That's one of the reasons why we
> have no distinction between matches and targets in nftables anymore.

That makes sense.
> Thanks.

These 2 MATCHSOCKMARK patches should be replaced with:
[PATCH nf-next] netfilter: xt_socket: add XT_SOCKET_RESTORESKMARK flag
[PATCH iptables] xt_socket: add --restore-skmark option

Thanks,
Harout