Skip to contentSkip to conversation

netfilter: nf_tables: add set_elem notifications

PATCH In review Arturo Borrero Gonzalez 3 participants 8 messages Mar 28, 2014

Subsystems (2)
Networking netfilter
lore
Share & tools
lore

Arturo Borrero Gonzalez

author
#0 lore raw

PATCH 1/1 netfilter: nf_tables: add set_elem notifications

This patch adds set_elems notifications.

When a set_elem is added/deleted, all listening peers in userspace will receive the corresponding notification.

Signed-off-by Arturo Borrero Gonzalez <[email protected]>
---
 net/netfilter/nf_tables_api.c |   68 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
net/netfilter/nf_tables_api.c +68 -0
index adce01e..33b1585 100644
@@ -2712,6 +2712,71 @@ static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb,
 	return -EOPNOTSUPP;
 }
 
+static int nf_tables_setelem_notify(const struct nft_ctx *ctx,
+				    const struct nft_set *set,
+				    const struct nft_set_elem *elem,
+				    int event, u16 flags)
+{
+	const struct sk_buff *oskb = ctx->skb;
+	struct sk_buff *skb;
+	struct net *net;
+	struct nfgenmsg *nfmsg;
+	struct nlmsghdr *nlh;
+	struct nlattr *nest;
+	bool report;
+	u32 portid;
+	u32 seq = ctx->nlh->nlmsg_seq;
+	int err;
+
+	portid = oskb ? NETLINK_CB(oskb).portid : 0;
+	net = oskb ? sock_net(oskb->sk) : &init_net;
+
+	report = ctx->nlh ? nlmsg_report(ctx->nlh) : false;
+	if (!report && !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
+		return 0;
+
+	err = -ENOBUFS;
+	skb = nlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (skb == NULL)
+		goto err;
+
+	event |= NFNL_SUBSYS_NFTABLES << 8;
+	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),
+			flags);
+	if (nlh == NULL)
+		goto err_free;
+
+	nfmsg = nlmsg_data(nlh);
+	nfmsg->nfgen_family	= ctx->afi->family;
+	nfmsg->version		= NFNETLINK_V0;
+	nfmsg->res_id		= 0;
+
+	if (nla_put_string(skb, NFTA_SET_TABLE, ctx->table->name))
+		goto err_free;
+	if (nla_put_string(skb, NFTA_SET_NAME, set->name))
+		goto err_free;
+
+	nest = nla_nest_start(skb, NFTA_SET_ELEM_LIST_ELEMENTS);
+	if (nest == NULL)
+		goto err_free;
+
+	err = nf_tables_fill_setelem(skb, set, elem);
+	if (err < 0)
+		goto err_free;
+
+	nla_nest_end(skb, nest);
+	nlmsg_end(skb, nlh);
+
+	err = nfnetlink_send(skb, net, portid, NFNLGRP_NFTABLES, report,
+			     GFP_KERNEL);
+err_free:
+	kfree_skb(skb);
+err:
+	if (err < 0)
+		nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, err);
+	return err;
+}
+
 static int nft_add_set_elem(const struct nft_ctx *ctx, struct nft_set *set,
 			    const struct nlattr *attr)
 {

@@ -2788,6 +2853,7 @@ static int nft_add_set_elem(const struct nft_ctx *ctx, struct nft_set *set,
 	if (err < 0)
 		goto err3;
 
+	nf_tables_setelem_notify(ctx, set, &elem, NFT_MSG_NEWSETELEM, 0);
 	return 0;
 
 err3:

@@ -2857,6 +2923,8 @@ static int nft_del_setelem(const struct nft_ctx *ctx, struct nft_set *set,
 
 	set->ops->remove(set, &elem);
 
+	nf_tables_setelem_notify(ctx, set, &elem, NFT_MSG_DELSETELEM, 0);
+
 	nft_data_uninit(&elem.key, NFT_DATA_VALUE);
 	if (set->flags & NFT_SET_MAP)
 		nft_data_uninit(&elem.data, set->dtype);
maintainer
#1 lore raw
Quoted 4 lines · Arturo Borrero Gonzalez

This patch adds set_elems notifications.

When a set_elem is added/deleted, all listening peers in userspace will receive the corresponding notification.

I was just looking into this now. Is this patch still crashing?

Quoted 99 lines
Signed-off-by Arturo Borrero Gonzalez <[email protected]>
---
 net/netfilter/nf_tables_api.c |   68 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
net/netfilter/nf_tables_api.c +68 -0
index adce01e..33b1585 100644
@@ -2712,6 +2712,71 @@ static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb,
 	return -EOPNOTSUPP;
 }
 
+static int nf_tables_setelem_notify(const struct nft_ctx *ctx,
+				    const struct nft_set *set,
+				    const struct nft_set_elem *elem,
+				    int event, u16 flags)
+{
+	const struct sk_buff *oskb = ctx->skb;
+	struct sk_buff *skb;
+	struct net *net;
+	struct nfgenmsg *nfmsg;
+	struct nlmsghdr *nlh;
+	struct nlattr *nest;
+	bool report;
+	u32 portid;
+	u32 seq = ctx->nlh->nlmsg_seq;
+	int err;
+
+	portid = oskb ? NETLINK_CB(oskb).portid : 0;
+	net = oskb ? sock_net(oskb->sk) : &init_net;
+
+	report = ctx->nlh ? nlmsg_report(ctx->nlh) : false;
+	if (!report && !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
+		return 0;
+
+	err = -ENOBUFS;
+	skb = nlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (skb == NULL)
+		goto err;
+
+	event |= NFNL_SUBSYS_NFTABLES << 8;
+	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),
+			flags);
+	if (nlh == NULL)
+		goto err_free;
+
+	nfmsg = nlmsg_data(nlh);
+	nfmsg->nfgen_family	= ctx->afi->family;
+	nfmsg->version		= NFNETLINK_V0;
+	nfmsg->res_id		= 0;
+
+	if (nla_put_string(skb, NFTA_SET_TABLE, ctx->table->name))
+		goto err_free;
+	if (nla_put_string(skb, NFTA_SET_NAME, set->name))
+		goto err_free;
+
+	nest = nla_nest_start(skb, NFTA_SET_ELEM_LIST_ELEMENTS);
+	if (nest == NULL)
+		goto err_free;
+
+	err = nf_tables_fill_setelem(skb, set, elem);
+	if (err < 0)
+		goto err_free;
+
+	nla_nest_end(skb, nest);
+	nlmsg_end(skb, nlh);
+
+	err = nfnetlink_send(skb, net, portid, NFNLGRP_NFTABLES, report,
+			     GFP_KERNEL);
+err_free:
+	kfree_skb(skb);
+err:
+	if (err < 0)
+		nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, err);
+	return err;
+}
+
 static int nft_add_set_elem(const struct nft_ctx *ctx, struct nft_set *set,
 			    const struct nlattr *attr)
 {

@@ -2788,6 +2853,7 @@ static int nft_add_set_elem(const struct nft_ctx *ctx, struct nft_set *set,
 	if (err < 0)
 		goto err3;
 
+	nf_tables_setelem_notify(ctx, set, &elem, NFT_MSG_NEWSETELEM, 0);
 	return 0;
 
 err3:

@@ -2857,6 +2923,8 @@ static int nft_del_setelem(const struct nft_ctx *ctx, struct nft_set *set,
 
 	set->ops->remove(set, &elem);
 
+	nf_tables_setelem_notify(ctx, set, &elem, NFT_MSG_DELSETELEM, 0);
+
 	nft_data_uninit(&elem.key, NFT_DATA_VALUE);
 	if (set->flags & NFT_SET_MAP)
 		nft_data_uninit(&elem.data, set->dtype);
author
#2 lore raw
Quoted 7 lines · Pablo Neira Ayuso
Quoted 4 lines · Arturo Borrero Gonzalez

This patch adds set_elems notifications.

When a set_elem is added/deleted, all listening peers in userspace will receive the corresponding notification.

I was just looking into this now. Is this patch still crashing?

Yes, still crashing :-(

Should this be delayed until the set batching is applied?

Signature · 5 lines
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#3 lore raw
Quoted 10 lines · Arturo Borrero Gonzalez
Quoted 7 lines · Pablo Neira Ayuso
Quoted 4 lines · Arturo Borrero Gonzalez

This patch adds set_elems notifications.

When a set_elem is added/deleted, all listening peers in userspace will receive the corresponding notification.

I was just looking into this now. Is this patch still crashing?

Yes, still crashing :-(

And this is only happening using anonymous sets?

Quoted 1 line

Should this be delayed until the set batching is applied?

I think it would be nice to get this in ASAP. Lets fix the crash and hope it doesn't clash much, but I don't think so.

#4 lore raw
In reply to Arturo Borrero Gonzalez
Quoted 4 lines · Arturo Borrero Gonzalez

This patch adds set_elems notifications.

When a set_elem is added/deleted, all listening peers in userspace will receive the corresponding notification.

So is the oops you observed with anonymous sets already resolved?

Quoted 14 lines
net/netfilter/nf_tables_api.c +10 -0
@@ -2712,6 +2712,71 @@ static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb,
 	return -EOPNOTSUPP;
 }
 
+static int nf_tables_setelem_notify(const struct nft_ctx *ctx,
+				    const struct nft_set *set,
+				    const struct nft_set_elem *elem,
+				    int event, u16 flags)
+{
+	const struct sk_buff *oskb = ctx->skb;
+	struct sk_buff *skb;
+	struct net *net;
+	struct nfgenmsg *nfmsg;
+	struct nlmsghdr *nlh;

net/netfilter/nf_tables_api.c static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb, · line 2734

Quoted 10 lines
net/netfilter/nf_tables_api.c +10 -0
+	struct nlattr *nest;
+	bool report;
+	u32 portid;
+	u32 seq = ctx->nlh->nlmsg_seq;
+	int err;
+
+	portid = oskb ? NETLINK_CB(oskb).portid : 0;
+	net = oskb ? sock_net(oskb->sk) : &init_net;
+
+	report = ctx->nlh ? nlmsg_report(ctx->nlh) : false;

All these tests seem overly defensive. We only have the codepath using nfnetlink to add or delete elements, so it should be very clear which context members are valid and which aren't. Also you're already using ctx->nlh before that for initialization of seq.

Quoted 1 line

From what I can tell, all these tests can be removed and the initialization

can be done unconditionally based on the context.

Quoted 21 lines
net/netfilter/nf_tables_api.c +21 -0
+	if (!report && !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
+		return 0;
+
+	err = -ENOBUFS;
+	skb = nlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (skb == NULL)
+		goto err;
+
+	event |= NFNL_SUBSYS_NFTABLES << 8;
+	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),
+			flags);
+	if (nlh == NULL)
+		goto err_free;
+
+	nfmsg = nlmsg_data(nlh);
+	nfmsg->nfgen_family	= ctx->afi->family;
+	nfmsg->version		= NFNETLINK_V0;
+	nfmsg->res_id		= 0;
+
+	if (nla_put_string(skb, NFTA_SET_TABLE, ctx->table->name))
+		goto err_free;

net/netfilter/nf_tables_api.c static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb, · line 2765

Quoted 10 lines
net/netfilter/nf_tables_api.c +10 -0
+	if (nla_put_string(skb, NFTA_SET_NAME, set->name))
+		goto err_free;
+
+	nest = nla_nest_start(skb, NFTA_SET_ELEM_LIST_ELEMENTS);
+	if (nest == NULL)
+		goto err_free;
+
+	err = nf_tables_fill_setelem(skb, set, elem);
+	if (err < 0)
+		goto err_free;

This single element per message is fine for now since we don't do proper atomic additions/removals, so we might fail mid way. But please make sure that userspace is able to handle lists containing multiple elements since this is way more efficient and should be changed once we do have proper atomic changes for set elements.

Quoted 7 lines
net/netfilter/nf_tables_api.c +7 -0
+
+	nla_nest_end(skb, nest);
+	nlmsg_end(skb, nlh);
+
+	err = nfnetlink_send(skb, net, portid, NFNLGRP_NFTABLES, report,
+			     GFP_KERNEL);
+err_free:

net/netfilter/nf_tables_api.c static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb, · line 2782

Quoted 10 lines
net/netfilter/nf_tables_api.c +7 -0
+	kfree_skb(skb);
+err:
+	if (err < 0)
+		nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, err);
+	return err;
+}
+
 static int nft_add_set_elem(const struct nft_ctx *ctx, struct nft_set *set,
 			    const struct nlattr *attr)
 {

Besides the things mentioned above, your patch looks fine to me.

#5 lore raw
In reply to Arturo Borrero Gonzalez

net/netfilter/nf_tables_api.c static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb, · line 2773

Quoted 10 lines · Arturo Borrero Gonzalez
net/netfilter/nf_tables_api.c +9 -0
+static int nf_tables_setelem_notify(const struct nft_ctx *ctx,
+				    const struct nft_set *set,
+				    const struct nft_set_elem *elem,
+				    int event, u16 flags)
+{
...
+	err = nfnetlink_send(skb, net, portid, NFNLGRP_NFTABLES, report,
+			     GFP_KERNEL);
+err_free:
+	kfree_skb(skb);

There's the reason of your crash. You must not free the skb after sending it.

maintainer
#6 lore raw
In reply to Arturo Borrero Gonzalez
Quoted 4 lines · Arturo Borrero Gonzalez

This patch adds set_elems notifications.

When a set_elem is added/deleted, all listening peers in userspace will receive the corresponding notification.

Now that Patrick discovered the problem, please address some more changes.

Quoted 47 lines
Signed-off-by Arturo Borrero Gonzalez <[email protected]>
---
 net/netfilter/nf_tables_api.c |   68 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)
net/netfilter/nf_tables_api.c +27 -0
index adce01e..33b1585 100644
@@ -2712,6 +2712,71 @@ static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb,
 	return -EOPNOTSUPP;
 }
 
+static int nf_tables_setelem_notify(const struct nft_ctx *ctx,
+				    const struct nft_set *set,
+				    const struct nft_set_elem *elem,
+				    int event, u16 flags)
+{
+	const struct sk_buff *oskb = ctx->skb;
+	struct sk_buff *skb;
+	struct net *net;
+	struct nfgenmsg *nfmsg;
+	struct nlmsghdr *nlh;
+	struct nlattr *nest;
+	bool report;
+	u32 portid;
+	u32 seq = ctx->nlh->nlmsg_seq;
+	int err;
+
+	portid = oskb ? NETLINK_CB(oskb).portid : 0;
+	net = oskb ? sock_net(oskb->sk) : &init_net;
+
+	report = ctx->nlh ? nlmsg_report(ctx->nlh) : false;
+	if (!report && !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
+		return 0;
+
+	err = -ENOBUFS;
+	skb = nlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (skb == NULL)
+		goto err;

From here:

net/netfilter/nf_tables_api.c +4 -0
+
+	event |= NFNL_SUBSYS_NFTABLES << 8;
+	nlh = nlmsg_put(skb, portid, seq, event, sizeof(struct nfgenmsg),
+			flags);

net/netfilter/nf_tables_api.c static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb, · line 2755

Quoted 10 lines
net/netfilter/nf_tables_api.c +10 -0
+	if (nlh == NULL)
+		goto err_free;
+
+	nfmsg = nlmsg_data(nlh);
+	nfmsg->nfgen_family	= ctx->afi->family;
+	nfmsg->version		= NFNETLINK_V0;
+	nfmsg->res_id		= 0;
+
+	if (nla_put_string(skb, NFTA_SET_TABLE, ctx->table->name))
+		goto err_free;

Please, use the "nla_put_failure" tag instead for consistency with other similar code.

net/netfilter/nf_tables_api.c static int nf_tables_getsetelem(struct sock *nlsk, struct sk_buff *skb, · line 2768

Quoted 13 lines
net/netfilter/nf_tables_api.c +13 -0
+	if (nla_put_string(skb, NFTA_SET_NAME, set->name))
+		goto err_free;
+
+	nest = nla_nest_start(skb, NFTA_SET_ELEM_LIST_ELEMENTS);
+	if (nest == NULL)
+		goto err_free;
+
+	err = nf_tables_fill_setelem(skb, set, elem);
+	if (err < 0)
+		goto err_free;
+
+	nla_nest_end(skb, nest);
+	nlmsg_end(skb, nlh);

To there, please move this code to the nf_tables_fill_setelem_info() function to make it consistent with what we already have, and get this function more maintainable by having in split in smaller chunks.

Thanks.

Quoted 32 lines
net/netfilter/nf_tables_api.c +14 -0
+
+	err = nfnetlink_send(skb, net, portid, NFNLGRP_NFTABLES, report,
+			     GFP_KERNEL);
+err_free:
+	kfree_skb(skb);
+err:
+	if (err < 0)
+		nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, err);
+	return err;
+}
+
 static int nft_add_set_elem(const struct nft_ctx *ctx, struct nft_set *set,
 			    const struct nlattr *attr)
 {

@@ -2788,6 +2853,7 @@ static int nft_add_set_elem(const struct nft_ctx *ctx, struct nft_set *set,
 	if (err < 0)
 		goto err3;
 
+	nf_tables_setelem_notify(ctx, set, &elem, NFT_MSG_NEWSETELEM, 0);
 	return 0;
 
 err3:

@@ -2857,6 +2923,8 @@ static int nft_del_setelem(const struct nft_ctx *ctx, struct nft_set *set,
 
 	set->ops->remove(set, &elem);
 
+	nf_tables_setelem_notify(ctx, set, &elem, NFT_MSG_DELSETELEM, 0);
+
 	nft_data_uninit(&elem.key, NFT_DATA_VALUE);
 	if (set->flags & NFT_SET_MAP)
 		nft_data_uninit(&elem.data, set->dtype);
author
#7 lore raw
Quoted 8 lines · Pablo Neira Ayuso
Quoted 4 lines · Arturo Borrero Gonzalez

This patch adds set_elems notifications.

When a set_elem is added/deleted, all listening peers in userspace will receive the corresponding notification.

Now that Patrick discovered the problem, please address some more changes.

Thanks Patrick, Pablo.

I will submit a new version shortly.

regards.

Signature · 5 lines
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html