netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] Netfilter fixes for net
@ 2020-01-31 19:24 Pablo Neira Ayuso
  2020-01-31 19:24 ` [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id Pablo Neira Ayuso
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-31 19:24 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

Hi,

The following patchset contains Netfilter fixes for net:

1) Fix suspicious RCU usage in ipset, from Jozsef Kadlecsik.

2) Use kvcalloc, from Joe Perches.

3) Flush flowtable hardware workqueue after garbage collection run,
   from Paul Blakey.

4) Missing flowtable hardware workqueue flush from nf_flow_table_free(),
   also from Paul.

5) Restore NF_FLOW_HW_DEAD in flow_offload_work_del(), from Paul.

6) Flowtable documentation fixes, from Matteo Croce.

You can pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git

Thank you.

----------------------------------------------------------------

The following changes since commit 44efc78d0e464ce70b45b165c005f8bedc17952e:

  net: mvneta: fix XDP support if sw bm is used as fallback (2020-01-29 13:57:59 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git HEAD

for you to fetch changes up to 78e06cf430934fc3768c342cbebdd1013dcd6fa7:

  netfilter: nf_flowtable: fix documentation (2020-01-31 19:31:42 +0100)

----------------------------------------------------------------
Joe Perches (1):
      netfilter: Use kvcalloc

Kadlecsik József (1):
      netfilter: ipset: fix suspicious RCU usage in find_set_and_id

Matteo Croce (1):
      netfilter: nf_flowtable: fix documentation

Paul Blakey (3):
      netfilter: flowtable: Fix hardware flush order on nf_flow_table_cleanup
      netfilter: flowtable: Fix missing flush hardware on table free
      netfilter: flowtable: Fix setting forgotten NF_FLOW_HW_DEAD flag

 Documentation/networking/nf_flowtable.txt |  2 +-
 net/netfilter/ipset/ip_set_core.c         | 41 ++++++++++++++++---------------
 net/netfilter/nf_conntrack_core.c         |  3 +--
 net/netfilter/nf_flow_table_core.c        |  3 ++-
 net/netfilter/nf_flow_table_offload.c     |  1 +
 net/netfilter/x_tables.c                  |  4 +--
 6 files changed, 28 insertions(+), 26 deletions(-)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id
  2020-01-31 19:24 [PATCH 0/6] Netfilter fixes for net Pablo Neira Ayuso
@ 2020-01-31 19:24 ` Pablo Neira Ayuso
  2020-02-01 20:57   ` Jakub Kicinski
  2020-01-31 19:24 ` [PATCH 2/6] netfilter: Use kvcalloc Pablo Neira Ayuso
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 12+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-31 19:24 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

From: Kadlecsik József <kadlec@blackhole.kfki.hu>

find_set_and_id() is called when the NFNL_SUBSYS_IPSET mutex is held.
However, in the error path there can be a follow-up recvmsg() without
the mutex held. Use the start() function of struct netlink_dump_control
instead of dump() to verify and report if the specified set does not
exist.

Thanks to Pablo Neira Ayuso for helping me to understand the subleties
of the netlink protocol.

Reported-by: syzbot+fc69d7cb21258ab4ae4d@syzkaller.appspotmail.com
Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/ipset/ip_set_core.c | 41 ++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index cf895bc80871..69c107f9ba8d 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -1483,31 +1483,34 @@ ip_set_dump_policy[IPSET_ATTR_CMD_MAX + 1] = {
 };
 
 static int
-dump_init(struct netlink_callback *cb, struct ip_set_net *inst)
+ip_set_dump_start(struct netlink_callback *cb)
 {
 	struct nlmsghdr *nlh = nlmsg_hdr(cb->skb);
 	int min_len = nlmsg_total_size(sizeof(struct nfgenmsg));
 	struct nlattr *cda[IPSET_ATTR_CMD_MAX + 1];
 	struct nlattr *attr = (void *)nlh + min_len;
+	struct sk_buff *skb = cb->skb;
+	struct ip_set_net *inst = ip_set_pernet(sock_net(skb->sk));
 	u32 dump_type;
-	ip_set_id_t index;
 	int ret;
 
 	ret = nla_parse(cda, IPSET_ATTR_CMD_MAX, attr,
 			nlh->nlmsg_len - min_len,
 			ip_set_dump_policy, NULL);
 	if (ret)
-		return ret;
+		goto error;
 
 	cb->args[IPSET_CB_PROTO] = nla_get_u8(cda[IPSET_ATTR_PROTOCOL]);
 	if (cda[IPSET_ATTR_SETNAME]) {
+		ip_set_id_t index;
 		struct ip_set *set;
 
 		set = find_set_and_id(inst, nla_data(cda[IPSET_ATTR_SETNAME]),
 				      &index);
-		if (!set)
-			return -ENOENT;
-
+		if (!set) {
+			ret = -ENOENT;
+			goto error;
+		}
 		dump_type = DUMP_ONE;
 		cb->args[IPSET_CB_INDEX] = index;
 	} else {
@@ -1523,10 +1526,17 @@ dump_init(struct netlink_callback *cb, struct ip_set_net *inst)
 	cb->args[IPSET_CB_DUMP] = dump_type;
 
 	return 0;
+
+error:
+	/* We have to create and send the error message manually :-( */
+	if (nlh->nlmsg_flags & NLM_F_ACK) {
+		netlink_ack(cb->skb, nlh, ret, NULL);
+	}
+	return ret;
 }
 
 static int
-ip_set_dump_start(struct sk_buff *skb, struct netlink_callback *cb)
+ip_set_dump_do(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	ip_set_id_t index = IPSET_INVALID_ID, max;
 	struct ip_set *set = NULL;
@@ -1537,18 +1547,8 @@ ip_set_dump_start(struct sk_buff *skb, struct netlink_callback *cb)
 	bool is_destroyed;
 	int ret = 0;
 
-	if (!cb->args[IPSET_CB_DUMP]) {
-		ret = dump_init(cb, inst);
-		if (ret < 0) {
-			nlh = nlmsg_hdr(cb->skb);
-			/* We have to create and send the error message
-			 * manually :-(
-			 */
-			if (nlh->nlmsg_flags & NLM_F_ACK)
-				netlink_ack(cb->skb, nlh, ret, NULL);
-			return ret;
-		}
-	}
+	if (!cb->args[IPSET_CB_DUMP])
+		return -EINVAL;
 
 	if (cb->args[IPSET_CB_INDEX] >= inst->ip_set_max)
 		goto out;
@@ -1684,7 +1684,8 @@ static int ip_set_dump(struct net *net, struct sock *ctnl, struct sk_buff *skb,
 
 	{
 		struct netlink_dump_control c = {
-			.dump = ip_set_dump_start,
+			.start = ip_set_dump_start,
+			.dump = ip_set_dump_do,
 			.done = ip_set_dump_done,
 		};
 		return netlink_dump_start(ctnl, skb, nlh, &c);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/6] netfilter: Use kvcalloc
  2020-01-31 19:24 [PATCH 0/6] Netfilter fixes for net Pablo Neira Ayuso
  2020-01-31 19:24 ` [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id Pablo Neira Ayuso
@ 2020-01-31 19:24 ` Pablo Neira Ayuso
  2020-01-31 19:24 ` [PATCH 3/6] netfilter: flowtable: Fix hardware flush order on nf_flow_table_cleanup Pablo Neira Ayuso
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-31 19:24 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

From: Joe Perches <joe@perches.com>

Convert the uses of kvmalloc_array with __GFP_ZERO to
the equivalent kvcalloc.

Signed-off-by: Joe Perches <joe@perches.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_core.c | 3 +--
 net/netfilter/x_tables.c          | 4 ++--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index f4c4b467c87e..d1305423640f 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -2248,8 +2248,7 @@ void *nf_ct_alloc_hashtable(unsigned int *sizep, int nulls)
 	BUILD_BUG_ON(sizeof(struct hlist_nulls_head) != sizeof(struct hlist_head));
 	nr_slots = *sizep = roundup(*sizep, PAGE_SIZE / sizeof(struct hlist_nulls_head));
 
-	hash = kvmalloc_array(nr_slots, sizeof(struct hlist_nulls_head),
-			      GFP_KERNEL | __GFP_ZERO);
+	hash = kvcalloc(nr_slots, sizeof(struct hlist_nulls_head), GFP_KERNEL);
 
 	if (hash && nulls)
 		for (i = 0; i < nr_slots; i++)
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index ce70c2576bb2..e27c6c5ba9df 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -939,14 +939,14 @@ EXPORT_SYMBOL(xt_check_entry_offsets);
  *
  * @size: number of entries
  *
- * Return: NULL or kmalloc'd or vmalloc'd array
+ * Return: NULL or zeroed kmalloc'd or vmalloc'd array
  */
 unsigned int *xt_alloc_entry_offsets(unsigned int size)
 {
 	if (size > XT_MAX_TABLE_SIZE / sizeof(unsigned int))
 		return NULL;
 
-	return kvmalloc_array(size, sizeof(unsigned int), GFP_KERNEL | __GFP_ZERO);
+	return kvcalloc(size, sizeof(unsigned int), GFP_KERNEL);
 
 }
 EXPORT_SYMBOL(xt_alloc_entry_offsets);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/6] netfilter: flowtable: Fix hardware flush order on nf_flow_table_cleanup
  2020-01-31 19:24 [PATCH 0/6] Netfilter fixes for net Pablo Neira Ayuso
  2020-01-31 19:24 ` [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id Pablo Neira Ayuso
  2020-01-31 19:24 ` [PATCH 2/6] netfilter: Use kvcalloc Pablo Neira Ayuso
@ 2020-01-31 19:24 ` Pablo Neira Ayuso
  2020-01-31 19:24 ` [PATCH 4/6] netfilter: flowtable: Fix missing flush hardware on table free Pablo Neira Ayuso
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-31 19:24 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

From: Paul Blakey <paulb@mellanox.com>

On netdev down event, nf_flow_table_cleanup() is called for the relevant
device and it cleans all the tables that are on that device.
If one of those tables has hardware offload flag,
nf_flow_table_iterate_cleanup flushes hardware and then runs the gc.
But the gc can queue more hardware work, which will take time to execute.

Instead first add the work, then flush it, to execute it now.

Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support")
Signed-off-by: Paul Blakey <paulb@mellanox.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 7e91989a1b55..14a069c72bb2 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -529,9 +529,9 @@ static void nf_flow_table_do_cleanup(struct flow_offload *flow, void *data)
 static void nf_flow_table_iterate_cleanup(struct nf_flowtable *flowtable,
 					  struct net_device *dev)
 {
-	nf_flow_table_offload_flush(flowtable);
 	nf_flow_table_iterate(flowtable, nf_flow_table_do_cleanup, dev);
 	flush_delayed_work(&flowtable->gc_work);
+	nf_flow_table_offload_flush(flowtable);
 }
 
 void nf_flow_table_cleanup(struct net_device *dev)
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/6] netfilter: flowtable: Fix missing flush hardware on table free
  2020-01-31 19:24 [PATCH 0/6] Netfilter fixes for net Pablo Neira Ayuso
                   ` (2 preceding siblings ...)
  2020-01-31 19:24 ` [PATCH 3/6] netfilter: flowtable: Fix hardware flush order on nf_flow_table_cleanup Pablo Neira Ayuso
@ 2020-01-31 19:24 ` Pablo Neira Ayuso
  2020-01-31 19:24 ` [PATCH 5/6] netfilter: flowtable: Fix setting forgotten NF_FLOW_HW_DEAD flag Pablo Neira Ayuso
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-31 19:24 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

From: Paul Blakey <paulb@mellanox.com>

If entries exist when freeing a hardware offload enabled table,
we queue work for hardware while running the gc iteration.

Execute it (flush) after queueing.

Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support")
Signed-off-by: Paul Blakey <paulb@mellanox.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 14a069c72bb2..8af28e10b4e6 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -553,6 +553,7 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
 	cancel_delayed_work_sync(&flow_table->gc_work);
 	nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
 	nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, flow_table);
+	nf_flow_table_offload_flush(flow_table);
 	rhashtable_destroy(&flow_table->rhashtable);
 }
 EXPORT_SYMBOL_GPL(nf_flow_table_free);
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 5/6] netfilter: flowtable: Fix setting forgotten NF_FLOW_HW_DEAD flag
  2020-01-31 19:24 [PATCH 0/6] Netfilter fixes for net Pablo Neira Ayuso
                   ` (3 preceding siblings ...)
  2020-01-31 19:24 ` [PATCH 4/6] netfilter: flowtable: Fix missing flush hardware on table free Pablo Neira Ayuso
@ 2020-01-31 19:24 ` Pablo Neira Ayuso
  2020-01-31 19:24 ` [PATCH 6/6] netfilter: nf_flowtable: fix documentation Pablo Neira Ayuso
  2020-02-01 20:59 ` [PATCH 0/6] Netfilter fixes for net Jakub Kicinski
  6 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-31 19:24 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

From: Paul Blakey <paulb@mellanox.com>

During the refactor this was accidently removed.

Fixes: ae29045018c8 ("netfilter: flowtable: add nf_flow_offload_tuple() helper")
Signed-off-by: Paul Blakey <paulb@mellanox.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_flow_table_offload.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index c8b70ffeef0c..83e1db37c3b0 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -675,6 +675,7 @@ static void flow_offload_work_del(struct flow_offload_work *offload)
 {
 	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_ORIGINAL);
 	flow_offload_tuple_del(offload, FLOW_OFFLOAD_DIR_REPLY);
+	set_bit(NF_FLOW_HW_DEAD, &offload->flow->flags);
 }
 
 static void flow_offload_tuple_stats(struct flow_offload_work *offload,
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 6/6] netfilter: nf_flowtable: fix documentation
  2020-01-31 19:24 [PATCH 0/6] Netfilter fixes for net Pablo Neira Ayuso
                   ` (4 preceding siblings ...)
  2020-01-31 19:24 ` [PATCH 5/6] netfilter: flowtable: Fix setting forgotten NF_FLOW_HW_DEAD flag Pablo Neira Ayuso
@ 2020-01-31 19:24 ` Pablo Neira Ayuso
  2020-02-01 20:59 ` [PATCH 0/6] Netfilter fixes for net Jakub Kicinski
  6 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2020-01-31 19:24 UTC (permalink / raw)
  To: netfilter-devel; +Cc: davem, netdev

From: Matteo Croce <mcroce@redhat.com>

In the flowtable documentation there is a missing semicolon, the command
as is would give this error:

    nftables.conf:5:27-33: Error: syntax error, unexpected devices, expecting newline or semicolon
                    hook ingress priority 0 devices = { br0, pppoe-data };
                                            ^^^^^^^
    nftables.conf:4:12-13: Error: invalid hook (null)
            flowtable ft {
                      ^^

Fixes: 19b351f16fd9 ("netfilter: add flowtable documentation")
Signed-off-by: Matteo Croce <mcroce@redhat.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 Documentation/networking/nf_flowtable.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/networking/nf_flowtable.txt b/Documentation/networking/nf_flowtable.txt
index ca2136c76042..0bf32d1121be 100644
--- a/Documentation/networking/nf_flowtable.txt
+++ b/Documentation/networking/nf_flowtable.txt
@@ -76,7 +76,7 @@ flowtable and add one rule to your forward chain.
 
         table inet x {
 		flowtable f {
-			hook ingress priority 0 devices = { eth0, eth1 };
+			hook ingress priority 0; devices = { eth0, eth1 };
 		}
                 chain y {
                         type filter hook forward priority 0; policy accept;
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id
  2020-01-31 19:24 ` [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id Pablo Neira Ayuso
@ 2020-02-01 20:57   ` Jakub Kicinski
  2020-02-01 21:05     ` Jakub Kicinski
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2020-02-01 20:57 UTC (permalink / raw)
  To: Pablo Neira Ayuso, Greg KH, Stephen Rothwell
  Cc: netfilter-devel, davem, netdev

On Fri, 31 Jan 2020 20:24:23 +0100, Pablo Neira Ayuso wrote:
> From: Kadlecsik József <kadlec@blackhole.kfki.hu>
> 
> find_set_and_id() is called when the NFNL_SUBSYS_IPSET mutex is held.
> However, in the error path there can be a follow-up recvmsg() without
> the mutex held. Use the start() function of struct netlink_dump_control
> instead of dump() to verify and report if the specified set does not
> exist.
> 
> Thanks to Pablo Neira Ayuso for helping me to understand the subleties
> of the netlink protocol.
> 
> Reported-by: syzbot+fc69d7cb21258ab4ae4d@syzkaller.appspotmail.com
> Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

This will trigger a missing signed-off-by check:

Commit 5038517119d5 ("netfilter: ipset: fix suspicious RCU usage in find_set_and_id")
	author Signed-off-by missing
	author email:    kadlec@blackhole.kfki.hu
	committer email: pablo@netfilter.org
	Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
	Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Problem is that the name differs by 'o' vs 'ó' (József Kadlecsik).

I wonder if it's worth getting rid of diacritics for the comparison..

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 0/6] Netfilter fixes for net
  2020-01-31 19:24 [PATCH 0/6] Netfilter fixes for net Pablo Neira Ayuso
                   ` (5 preceding siblings ...)
  2020-01-31 19:24 ` [PATCH 6/6] netfilter: nf_flowtable: fix documentation Pablo Neira Ayuso
@ 2020-02-01 20:59 ` Jakub Kicinski
  6 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2020-02-01 20:59 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, davem, netdev

On Fri, 31 Jan 2020 20:24:22 +0100, Pablo Neira Ayuso wrote:
> Hi,
> 
> The following patchset contains Netfilter fixes for net:
> 
> 1) Fix suspicious RCU usage in ipset, from Jozsef Kadlecsik.
> 
> 2) Use kvcalloc, from Joe Perches.
> 
> 3) Flush flowtable hardware workqueue after garbage collection run,
>    from Paul Blakey.
> 
> 4) Missing flowtable hardware workqueue flush from nf_flow_table_free(),
>    also from Paul.
> 
> 5) Restore NF_FLOW_HW_DEAD in flow_offload_work_del(), from Paul.
> 
> 6) Flowtable documentation fixes, from Matteo Croce.

Pulled, thanks!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id
  2020-02-01 20:57   ` Jakub Kicinski
@ 2020-02-01 21:05     ` Jakub Kicinski
  2020-02-01 21:11       ` Jozsef Kadlecsik
  0 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2020-02-01 21:05 UTC (permalink / raw)
  To: Pablo Neira Ayuso, Greg KH, Stephen Rothwell
  Cc: netfilter-devel, davem, netdev

On Sat, 1 Feb 2020 12:57:36 -0800, Jakub Kicinski wrote:
> On Fri, 31 Jan 2020 20:24:23 +0100, Pablo Neira Ayuso wrote:
> > From: Kadlecsik József <kadlec@blackhole.kfki.hu>
> > 
> > find_set_and_id() is called when the NFNL_SUBSYS_IPSET mutex is held.
> > However, in the error path there can be a follow-up recvmsg() without
> > the mutex held. Use the start() function of struct netlink_dump_control
> > instead of dump() to verify and report if the specified set does not
> > exist.
> > 
> > Thanks to Pablo Neira Ayuso for helping me to understand the subleties
> > of the netlink protocol.
> > 
> > Reported-by: syzbot+fc69d7cb21258ab4ae4d@syzkaller.appspotmail.com
> > Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>  
> 
> This will trigger a missing signed-off-by check:
> 
> Commit 5038517119d5 ("netfilter: ipset: fix suspicious RCU usage in find_set_and_id")
> 	author Signed-off-by missing
> 	author email:    kadlec@blackhole.kfki.hu
> 	committer email: pablo@netfilter.org
> 	Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> 	Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> 
> Problem is that the name differs by 'o' vs 'ó' (József Kadlecsik).
> 
> I wonder if it's worth getting rid of diacritics for the comparison..

Mm.. also the name and surname are the other way around :S

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id
  2020-02-01 21:05     ` Jakub Kicinski
@ 2020-02-01 21:11       ` Jozsef Kadlecsik
  2020-02-02  4:21         ` Jakub Kicinski
  0 siblings, 1 reply; 12+ messages in thread
From: Jozsef Kadlecsik @ 2020-02-01 21:11 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Pablo Neira Ayuso, Greg KH, Stephen Rothwell, netfilter-devel,
	David Miller, netdev

[-- Attachment #1: Type: text/plain, Size: 1936 bytes --]

On Sat, 1 Feb 2020, Jakub Kicinski wrote:

> On Sat, 1 Feb 2020 12:57:36 -0800, Jakub Kicinski wrote:
> > On Fri, 31 Jan 2020 20:24:23 +0100, Pablo Neira Ayuso wrote:
> > > From: Kadlecsik József <kadlec@blackhole.kfki.hu>
> > > 
> > > find_set_and_id() is called when the NFNL_SUBSYS_IPSET mutex is held.
> > > However, in the error path there can be a follow-up recvmsg() without
> > > the mutex held. Use the start() function of struct netlink_dump_control
> > > instead of dump() to verify and report if the specified set does not
> > > exist.
> > > 
> > > Thanks to Pablo Neira Ayuso for helping me to understand the subleties
> > > of the netlink protocol.
> > > 
> > > Reported-by: syzbot+fc69d7cb21258ab4ae4d@syzkaller.appspotmail.com
> > > Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>  
> > 
> > This will trigger a missing signed-off-by check:
> > 
> > Commit 5038517119d5 ("netfilter: ipset: fix suspicious RCU usage in find_set_and_id")
> > 	author Signed-off-by missing
> > 	author email:    kadlec@blackhole.kfki.hu
> > 	committer email: pablo@netfilter.org
> > 	Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> > 	Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > 
> > Problem is that the name differs by 'o' vs 'ó' (József Kadlecsik).
> > 
> > I wonder if it's worth getting rid of diacritics for the comparison..
> 
> Mm.. also the name and surname are the other way around :S

Oh, my... Hungarian names... :-) But I also should set my sender email 
address to kadlec@netfilter.org.

What's the best approach for this patch? Shall I resend it?

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics
          H-1525 Budapest 114, POB. 49, Hungary

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id
  2020-02-01 21:11       ` Jozsef Kadlecsik
@ 2020-02-02  4:21         ` Jakub Kicinski
  0 siblings, 0 replies; 12+ messages in thread
From: Jakub Kicinski @ 2020-02-02  4:21 UTC (permalink / raw)
  To: Jozsef Kadlecsik
  Cc: Pablo Neira Ayuso, Greg KH, Stephen Rothwell, netfilter-devel,
	David Miller, netdev

On Sat, 1 Feb 2020 22:11:04 +0100 (CET), Jozsef Kadlecsik wrote:
> On Sat, 1 Feb 2020, Jakub Kicinski wrote:
> > On Sat, 1 Feb 2020 12:57:36 -0800, Jakub Kicinski wrote:  
> > > On Fri, 31 Jan 2020 20:24:23 +0100, Pablo Neira Ayuso wrote:  
> > > > From: Kadlecsik József <kadlec@blackhole.kfki.hu>
> > > > 
> > > > find_set_and_id() is called when the NFNL_SUBSYS_IPSET mutex is held.
> > > > However, in the error path there can be a follow-up recvmsg() without
> > > > the mutex held. Use the start() function of struct netlink_dump_control
> > > > instead of dump() to verify and report if the specified set does not
> > > > exist.
> > > > 
> > > > Thanks to Pablo Neira Ayuso for helping me to understand the subleties
> > > > of the netlink protocol.
> > > > 
> > > > Reported-by: syzbot+fc69d7cb21258ab4ae4d@syzkaller.appspotmail.com
> > > > Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>    
> > > 
> > > This will trigger a missing signed-off-by check:
> > > 
> > > Commit 5038517119d5 ("netfilter: ipset: fix suspicious RCU usage in find_set_and_id")
> > > 	author Signed-off-by missing
> > > 	author email:    kadlec@blackhole.kfki.hu
> > > 	committer email: pablo@netfilter.org
> > > 	Signed-off-by: Jozsef Kadlecsik <kadlec@netfilter.org>
> > > 	Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > > 
> > > Problem is that the name differs by 'o' vs 'ó' (József Kadlecsik).
> > > 
> > > I wonder if it's worth getting rid of diacritics for the comparison..  
> > 
> > Mm.. also the name and surname are the other way around :S  
> 
> Oh, my... Hungarian names... :-) But I also should set my sender email 
> address to kadlec@netfilter.org.
> 
> What's the best approach for this patch? Shall I resend it?

This one is in David Miller's tree already so too late, just fix up
your setup going forward please.

To a fellow Central European Jozsef Kadlecsik and Kadlecsik József 
are more than interchangeable, but probably not worth teaching the
validation scripts about this.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-02-02  4:21 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 19:24 [PATCH 0/6] Netfilter fixes for net Pablo Neira Ayuso
2020-01-31 19:24 ` [PATCH 1/6] netfilter: ipset: fix suspicious RCU usage in find_set_and_id Pablo Neira Ayuso
2020-02-01 20:57   ` Jakub Kicinski
2020-02-01 21:05     ` Jakub Kicinski
2020-02-01 21:11       ` Jozsef Kadlecsik
2020-02-02  4:21         ` Jakub Kicinski
2020-01-31 19:24 ` [PATCH 2/6] netfilter: Use kvcalloc Pablo Neira Ayuso
2020-01-31 19:24 ` [PATCH 3/6] netfilter: flowtable: Fix hardware flush order on nf_flow_table_cleanup Pablo Neira Ayuso
2020-01-31 19:24 ` [PATCH 4/6] netfilter: flowtable: Fix missing flush hardware on table free Pablo Neira Ayuso
2020-01-31 19:24 ` [PATCH 5/6] netfilter: flowtable: Fix setting forgotten NF_FLOW_HW_DEAD flag Pablo Neira Ayuso
2020-01-31 19:24 ` [PATCH 6/6] netfilter: nf_flowtable: fix documentation Pablo Neira Ayuso
2020-02-01 20:59 ` [PATCH 0/6] Netfilter fixes for net Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).