An skb can be added to a neigh->arp_queue while waiting for an arp
reply. Where original skb's skb->dev can be different to neigh's
neigh->dev. For instance in case of bridging dnated skb from one veth to
another, the skb would be added to a neigh->arp_queue of the bridge.
There is no explicit mechanism that prevents the original skb->dev link
of such skb from being freed under us. For instance neigh_flush_dev does
not cleanup skbs from different device's neigh queue. But that original
link can be used and lead to crash on e.g. this stack:
arp_process
neigh_update
skb = __skb_dequeue(&neigh->arp_queue)
neigh_resolve_output(..., skb)
...
br_nf_dev_xmit
br_nf_pre_routing_finish_bridge_slow
skb->dev = nf_bridge->physindev
br_handle_frame_finish
So let's improve neigh_flush_dev to also purge skbs when device
equal to their skb->nf_bridge->physindev gets destroyed.
Signed-off-by: Pavel Tikhomirov <[email protected]>
---
I'm not fully sure, but likely it is:
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
---
net/core/neighbour.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 552719c3bbc3d..47d2d52f17da3 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -39,6 +39,9 @@
#include <linux/inetdevice.h>
#include <net/addrconf.h>
+#include <linux/skbuff.h>
+#include <linux/netfilter_bridge.h>
+
#include <trace/events/neigh.h>
#define NEIGH_DEBUG 1
@@ -377,6 +380,28 @@ static void pneigh_queue_purge(struct sk_buff_head *list, struct net *net,
}
}
+static void neigh_purge_nf_bridge_dev(struct neighbour *neigh, struct net_device *dev)
+{
+ struct sk_buff_head *list = &neigh->arp_queue;
+ struct nf_bridge_info *nf_bridge;
+ struct sk_buff *skb, *next;
+
+ write_lock(&neigh->lock);
+ skb = skb_peek(list);
+ while (skb) {
+ nf_bridge = nf_bridge_info_get(skb);
+
+ next = skb_peek_next(skb, list);
+ if (nf_bridge && nf_bridge->physindev == dev) {
+ __skb_unlink(skb, list);
+ neigh->arp_queue_len_bytes -= skb->truesize;
+ kfree_skb(skb);
+ }
+ skb = next;
+ }
+ write_unlock(&neigh->lock);
+}
+
static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
bool skip_perm)
{
@@ -393,6 +418,7 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
while ((n = rcu_dereference_protected(*np,
lockdep_is_held(&tbl->lock))) != NULL) {
if (dev && n->dev != dev) {
+ neigh_purge_nf_bridge_dev(n, dev);
np = &n->next;
continue;
}
--
2.43.0
On Mon, Jan 8, 2024 at 9:52 AM Pavel Tikhomirov
<[email protected]> wrote:
>
> An skb can be added to a neigh->arp_queue while waiting for an arp
> reply. Where original skb's skb->dev can be different to neigh's
> neigh->dev. For instance in case of bridging dnated skb from one veth to
> another, the skb would be added to a neigh->arp_queue of the bridge.
>
> There is no explicit mechanism that prevents the original skb->dev link
> of such skb from being freed under us. For instance neigh_flush_dev does
> not cleanup skbs from different device's neigh queue. But that original
> link can be used and lead to crash on e.g. this stack:
>
> arp_process
> neigh_update
> skb = __skb_dequeue(&neigh->arp_queue)
> neigh_resolve_output(..., skb)
> ...
> br_nf_dev_xmit
> br_nf_pre_routing_finish_bridge_slow
> skb->dev = nf_bridge->physindev
> br_handle_frame_finish
>
> So let's improve neigh_flush_dev to also purge skbs when device
> equal to their skb->nf_bridge->physindev gets destroyed.
>
> Signed-off-by: Pavel Tikhomirov <[email protected]>
> ---
> I'm not fully sure, but likely it is:
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> ---
> net/core/neighbour.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 552719c3bbc3d..47d2d52f17da3 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -39,6 +39,9 @@
> #include <linux/inetdevice.h>
> #include <net/addrconf.h>
>
> +#include <linux/skbuff.h>
> +#include <linux/netfilter_bridge.h>
> +
> #include <trace/events/neigh.h>
>
> #define NEIGH_DEBUG 1
> @@ -377,6 +380,28 @@ static void pneigh_queue_purge(struct sk_buff_head *list, struct net *net,
> }
> }
>
> +static void neigh_purge_nf_bridge_dev(struct neighbour *neigh, struct net_device *dev)
> +{
> + struct sk_buff_head *list = &neigh->arp_queue;
> + struct nf_bridge_info *nf_bridge;
> + struct sk_buff *skb, *next;
> +
> + write_lock(&neigh->lock);
> + skb = skb_peek(list);
> + while (skb) {
> + nf_bridge = nf_bridge_info_get(skb);
This depends on CONFIG_BRIDGE_NETFILTER
Can we solve this issue without adding another layer violation ?
> +
> + next = skb_peek_next(skb, list);
> + if (nf_bridge && nf_bridge->physindev == dev) {
> + __skb_unlink(skb, list);
> + neigh->arp_queue_len_bytes -= skb->truesize;
> + kfree_skb(skb);
> + }
> + skb = next;
> + }
> + write_unlock(&neigh->lock);
> +}
> +
> static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
> bool skip_perm)
> {
> @@ -393,6 +418,7 @@ static void neigh_flush_dev(struct neigh_table *tbl, struct net_device *dev,
> while ((n = rcu_dereference_protected(*np,
> lockdep_is_held(&tbl->lock))) != NULL) {
> if (dev && n->dev != dev) {
> + neigh_purge_nf_bridge_dev(n, dev);
> np = &n->next;
> continue;
> }
> --
> 2.43.0
>
Pavel Tikhomirov <[email protected]> wrote:
> An skb can be added to a neigh->arp_queue while waiting for an arp
> reply. Where original skb's skb->dev can be different to neigh's
> neigh->dev. For instance in case of bridging dnated skb from one veth to
> another, the skb would be added to a neigh->arp_queue of the bridge.
>
> There is no explicit mechanism that prevents the original skb->dev link
> of such skb from being freed under us. For instance neigh_flush_dev does
> not cleanup skbs from different device's neigh queue. But that original
> link can be used and lead to crash on e.g. this stack:
>
> arp_process
> neigh_update
> skb = __skb_dequeue(&neigh->arp_queue)
> neigh_resolve_output(..., skb)
> ...
> br_nf_dev_xmit
> br_nf_pre_routing_finish_bridge_slow
> skb->dev = nf_bridge->physindev
> br_handle_frame_finish
>
> So let's improve neigh_flush_dev to also purge skbs when device
> equal to their skb->nf_bridge->physindev gets destroyed.
Can we fix this by replacing physindev pointer with plain
ifindex instead? There are not too many places that need to
peek into the original net_device struct, so I don't think
the additional dev_get_by_index_rcu() would be an issue.
On 08/01/2024 19:15, Florian Westphal wrote:
> Pavel Tikhomirov <[email protected]> wrote:
>> An skb can be added to a neigh->arp_queue while waiting for an arp
>> reply. Where original skb's skb->dev can be different to neigh's
>> neigh->dev. For instance in case of bridging dnated skb from one veth to
>> another, the skb would be added to a neigh->arp_queue of the bridge.
>>
>> There is no explicit mechanism that prevents the original skb->dev link
>> of such skb from being freed under us. For instance neigh_flush_dev does
>> not cleanup skbs from different device's neigh queue. But that original
>> link can be used and lead to crash on e.g. this stack:
>>
>> arp_process
>> neigh_update
>> skb = __skb_dequeue(&neigh->arp_queue)
>> neigh_resolve_output(..., skb)
>> ...
>> br_nf_dev_xmit
>> br_nf_pre_routing_finish_bridge_slow
>> skb->dev = nf_bridge->physindev
>> br_handle_frame_finish
>>
>> So let's improve neigh_flush_dev to also purge skbs when device
>> equal to their skb->nf_bridge->physindev gets destroyed.
>
> Can we fix this by replacing physindev pointer with plain
> ifindex instead? There are not too many places that need to
> peek into the original net_device struct, so I don't think
> the additional dev_get_by_index_rcu() would be an issue.
I will work on it, thanks for a good idea!
--
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.
On 08/01/2024 19:26, Pavel Tikhomirov wrote:
>
>
> On 08/01/2024 19:15, Florian Westphal wrote:
>> Pavel Tikhomirov <[email protected]> wrote:
>>> An skb can be added to a neigh->arp_queue while waiting for an arp
>>> reply. Where original skb's skb->dev can be different to neigh's
>>> neigh->dev. For instance in case of bridging dnated skb from one veth to
>>> another, the skb would be added to a neigh->arp_queue of the bridge.
>>>
>>> There is no explicit mechanism that prevents the original skb->dev link
>>> of such skb from being freed under us. For instance neigh_flush_dev does
>>> not cleanup skbs from different device's neigh queue. But that original
>>> link can be used and lead to crash on e.g. this stack:
>>>
>>> arp_process
>>> neigh_update
>>> skb = __skb_dequeue(&neigh->arp_queue)
>>> neigh_resolve_output(..., skb)
>>> ...
>>> br_nf_dev_xmit
>>> br_nf_pre_routing_finish_bridge_slow
>>> skb->dev = nf_bridge->physindev
>>> br_handle_frame_finish
>>>
>>> So let's improve neigh_flush_dev to also purge skbs when device
>>> equal to their skb->nf_bridge->physindev gets destroyed.
>>
>> Can we fix this by replacing physindev pointer with plain
>> ifindex instead? There are not too many places that need to
>> peek into the original net_device struct, so I don't think
>> the additional dev_get_by_index_rcu() would be an issue.
>
> I will work on it, thanks for a good idea!
>
If we replace nf_bridge->physindev completely, we would need to do
something like this in every place physindev was used:
diff --git a/include/linux/netfilter_bridge.h
b/include/linux/netfilter_bridge.h
index f980edfdd2783..105fbdb029261 100644
--- a/include/linux/netfilter_bridge.h
+++ b/include/linux/netfilter_bridge.h
@@ -56,11 +56,15 @@ static inline int nf_bridge_get_physoutif(const
struct sk_buff *skb)
}
static inline struct net_device *
-nf_bridge_get_physindev(const struct sk_buff *skb)
+nf_bridge_get_physindev_rcu(const struct sk_buff *skb)
{
const struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
+ struct net_device *dev;
- return nf_bridge ? nf_bridge->physindev : NULL;
+ if (!nf_bridge || !skb->dev)
+ return 0;
+
+ return dev_get_by_index_rcu(skb->dev->net, nf_bridge->physindev_if);
}
static inline struct net_device *
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a5ae952454c89..51e7cdf9b51c9 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -295,7 +295,7 @@ struct nf_bridge_info {
u8 bridged_dnat:1;
u8 sabotage_in_done:1;
__u16 frag_max_size;
- struct net_device *physindev;
+ int *physindev_if;
/* always valid & non-NULL from FORWARD on, for physdev match */
struct net_device *physoutdev;
diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c
b/net/ipv4/netfilter/nf_reject_ipv4.c
index f01b038fc1cda..01b3eb169772e 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -289,7 +289,8 @@ void nf_send_reset(struct net *net, struct sock *sk,
struct sk_buff *oldskb,
* build the eth header using the original destination's MAC as the
* source, and send the RST packet directly.
*/
- br_indev = nf_bridge_get_physindev(oldskb);
+ rcu_read_lock_bh();
+ br_indev = nf_bridge_get_physindev_rcu(oldskb);
if (br_indev) {
struct ethhdr *oeth = eth_hdr(oldskb);
@@ -297,12 +298,19 @@ void nf_send_reset(struct net *net, struct sock
*sk, struct sk_buff *oldskb,
niph->tot_len = htons(nskb->len);
ip_send_check(niph);
if (dev_hard_header(nskb, nskb->dev, ntohs(nskb->protocol),
- oeth->h_source, oeth->h_dest,
nskb->len) < 0)
+ oeth->h_source, oeth->h_dest,
nskb->len) < 0) {
+ rcu_read_unlock_bh();
goto free_nskb;
+ }
dev_queue_xmit(nskb);
- } else
+ rcu_read_unlock_bh();
+ } else {
+ rcu_read_unlock_bh();
#endif
ip_local_out(net, nskb->sk, nskb);
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+ }
+#endif
return;
Does it sound good?
Or maybe instead we can have extra physindev_if field in addition to
existing physindev to only do dev_get_by_index_rcu inside
br_nf_pre_routing_finish_bridge_slow to doublecheck the ->physindev link?
Sorry in advance if I'm missing anything obvious.
--
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.
Hi Pavel,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
[also build test WARNING on net/main linus/master horms-ipvs/master v6.7 next-20240108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Pavel-Tikhomirov/neighbour-purge-nf_bridged-skb-from-foreign-device-neigh/20240108-165551
base: net-next/main
patch link: https://lore.kernel.org/r/20240108085232.95437-1-ptikhomirov%40virtuozzo.com
patch subject: [PATCH] neighbour: purge nf_bridged skb from foreign device neigh
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240109/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240109/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
net/core/neighbour.c: In function 'neigh_purge_nf_bridge_dev':
net/core/neighbour.c:392:29: error: implicit declaration of function 'nf_bridge_info_get' [-Werror=implicit-function-declaration]
392 | nf_bridge = nf_bridge_info_get(skb);
| ^~~~~~~~~~~~~~~~~~
>> net/core/neighbour.c:392:27: warning: assignment to 'struct nf_bridge_info *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
392 | nf_bridge = nf_bridge_info_get(skb);
| ^
net/core/neighbour.c:395:43: error: invalid use of undefined type 'struct nf_bridge_info'
395 | if (nf_bridge && nf_bridge->physindev == dev) {
| ^~
cc1: some warnings being treated as errors
vim +392 net/core/neighbour.c
382
383 static void neigh_purge_nf_bridge_dev(struct neighbour *neigh, struct net_device *dev)
384 {
385 struct sk_buff_head *list = &neigh->arp_queue;
386 struct nf_bridge_info *nf_bridge;
387 struct sk_buff *skb, *next;
388
389 write_lock(&neigh->lock);
390 skb = skb_peek(list);
391 while (skb) {
> 392 nf_bridge = nf_bridge_info_get(skb);
393
394 next = skb_peek_next(skb, list);
395 if (nf_bridge && nf_bridge->physindev == dev) {
396 __skb_unlink(skb, list);
397 neigh->arp_queue_len_bytes -= skb->truesize;
398 kfree_skb(skb);
399 }
400 skb = next;
401 }
402 write_unlock(&neigh->lock);
403 }
404
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
That problem happens because the patch is not ready to handle the lack
of CONFIG_BRIDGE_NETFILTER (as Eric already mentioned earlier in this
thread).
On 09/01/2024 13:38, kernel test robot wrote:
> Hi Pavel,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on net-next/main]
> [also build test WARNING on net/main linus/master horms-ipvs/master v6.7 next-20240108]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Pavel-Tikhomirov/neighbour-purge-nf_bridged-skb-from-foreign-device-neigh/20240108-165551
> base: net-next/main
> patch link: https://lore.kernel.org/r/20240108085232.95437-1-ptikhomirov%40virtuozzo.com
> patch subject: [PATCH] neighbour: purge nf_bridged skb from foreign device neigh
> config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240109/[email protected]/config)
> compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240109/[email protected]/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <[email protected]>
> | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> All warnings (new ones prefixed by >>):
>
> net/core/neighbour.c: In function 'neigh_purge_nf_bridge_dev':
> net/core/neighbour.c:392:29: error: implicit declaration of function 'nf_bridge_info_get' [-Werror=implicit-function-declaration]
> 392 | nf_bridge = nf_bridge_info_get(skb);
> | ^~~~~~~~~~~~~~~~~~
>>> net/core/neighbour.c:392:27: warning: assignment to 'struct nf_bridge_info *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
> 392 | nf_bridge = nf_bridge_info_get(skb);
> | ^
> net/core/neighbour.c:395:43: error: invalid use of undefined type 'struct nf_bridge_info'
> 395 | if (nf_bridge && nf_bridge->physindev == dev) {
> | ^~
> cc1: some warnings being treated as errors
>
>
> vim +392 net/core/neighbour.c
>
> 382
> 383 static void neigh_purge_nf_bridge_dev(struct neighbour *neigh, struct net_device *dev)
> 384 {
> 385 struct sk_buff_head *list = &neigh->arp_queue;
> 386 struct nf_bridge_info *nf_bridge;
> 387 struct sk_buff *skb, *next;
> 388
> 389 write_lock(&neigh->lock);
> 390 skb = skb_peek(list);
> 391 while (skb) {
> > 392 nf_bridge = nf_bridge_info_get(skb);
> 393
> 394 next = skb_peek_next(skb, list);
> 395 if (nf_bridge && nf_bridge->physindev == dev) {
> 396 __skb_unlink(skb, list);
> 397 neigh->arp_queue_len_bytes -= skb->truesize;
> 398 kfree_skb(skb);
> 399 }
> 400 skb = next;
> 401 }
> 402 write_unlock(&neigh->lock);
> 403 }
> 404
>
--
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.
Hi Pavel,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
[also build test WARNING on net/main linus/master horms-ipvs/master v6.7 next-20240109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Pavel-Tikhomirov/neighbour-purge-nf_bridged-skb-from-foreign-device-neigh/20240108-165551
base: net-next/main
patch link: https://lore.kernel.org/r/20240108085232.95437-1-ptikhomirov%40virtuozzo.com
patch subject: [PATCH] neighbour: purge nf_bridged skb from foreign device neigh
config: i386-defconfig (https://download.01.org/0day-ci/archive/20240109/[email protected]/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240109/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
net/core/neighbour.c: In function 'neigh_purge_nf_bridge_dev':
net/core/neighbour.c:392:15: error: implicit declaration of function 'nf_bridge_info_get'; did you mean 'nf_bridge_in_prerouting'? [-Werror=implicit-function-declaration]
nf_bridge = nf_bridge_info_get(skb);
^~~~~~~~~~~~~~~~~~
nf_bridge_in_prerouting
>> net/core/neighbour.c:392:13: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
nf_bridge = nf_bridge_info_get(skb);
^
net/core/neighbour.c:395:29: error: dereferencing pointer to incomplete type 'struct nf_bridge_info'
if (nf_bridge && nf_bridge->physindev == dev) {
^~
cc1: some warnings being treated as errors
vim +392 net/core/neighbour.c
382
383 static void neigh_purge_nf_bridge_dev(struct neighbour *neigh, struct net_device *dev)
384 {
385 struct sk_buff_head *list = &neigh->arp_queue;
386 struct nf_bridge_info *nf_bridge;
387 struct sk_buff *skb, *next;
388
389 write_lock(&neigh->lock);
390 skb = skb_peek(list);
391 while (skb) {
> 392 nf_bridge = nf_bridge_info_get(skb);
393
394 next = skb_peek_next(skb, list);
395 if (nf_bridge && nf_bridge->physindev == dev) {
396 __skb_unlink(skb, list);
397 neigh->arp_queue_len_bytes -= skb->truesize;
398 kfree_skb(skb);
399 }
400 skb = next;
401 }
402 write_unlock(&neigh->lock);
403 }
404
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Pavel,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
[also build test ERROR on net/main linus/master horms-ipvs/master v6.7 next-20240109]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Pavel-Tikhomirov/neighbour-purge-nf_bridged-skb-from-foreign-device-neigh/20240108-165551
base: net-next/main
patch link: https://lore.kernel.org/r/20240108085232.95437-1-ptikhomirov%40virtuozzo.com
patch subject: [PATCH] neighbour: purge nf_bridged skb from foreign device neigh
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240109/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240109/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
net/core/neighbour.c: In function 'neigh_purge_nf_bridge_dev':
>> net/core/neighbour.c:392:29: error: implicit declaration of function 'nf_bridge_info_get' [-Werror=implicit-function-declaration]
392 | nf_bridge = nf_bridge_info_get(skb);
| ^~~~~~~~~~~~~~~~~~
net/core/neighbour.c:392:27: warning: assignment to 'struct nf_bridge_info *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
392 | nf_bridge = nf_bridge_info_get(skb);
| ^
>> net/core/neighbour.c:395:43: error: invalid use of undefined type 'struct nf_bridge_info'
395 | if (nf_bridge && nf_bridge->physindev == dev) {
| ^~
cc1: some warnings being treated as errors
vim +/nf_bridge_info_get +392 net/core/neighbour.c
382
383 static void neigh_purge_nf_bridge_dev(struct neighbour *neigh, struct net_device *dev)
384 {
385 struct sk_buff_head *list = &neigh->arp_queue;
386 struct nf_bridge_info *nf_bridge;
387 struct sk_buff *skb, *next;
388
389 write_lock(&neigh->lock);
390 skb = skb_peek(list);
391 while (skb) {
> 392 nf_bridge = nf_bridge_info_get(skb);
393
394 next = skb_peek_next(skb, list);
> 395 if (nf_bridge && nf_bridge->physindev == dev) {
396 __skb_unlink(skb, list);
397 neigh->arp_queue_len_bytes -= skb->truesize;
398 kfree_skb(skb);
399 }
400 skb = next;
401 }
402 write_unlock(&neigh->lock);
403 }
404
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Pavel Tikhomirov <[email protected]> wrote:
> index f980edfdd2783..105fbdb029261 100644
> --- a/include/linux/netfilter_bridge.h
> +++ b/include/linux/netfilter_bridge.h
> @@ -56,11 +56,15 @@ static inline int nf_bridge_get_physoutif(const struct
> sk_buff *skb)
> }
>
> static inline struct net_device *
> -nf_bridge_get_physindev(const struct sk_buff *skb)
> +nf_bridge_get_physindev_rcu(const struct sk_buff *skb)
> {
> const struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
> + struct net_device *dev;
>
> - return nf_bridge ? nf_bridge->physindev : NULL;
> + if (!nf_bridge || !skb->dev)
> + return 0;
> +
> + return dev_get_by_index_rcu(skb->dev->net, nf_bridge->physindev_if);
You could use dev_net(skb->dev), yes.
Or create a preparation patch that does:
-nf_bridge_get_physindev(const struct sk_buff *skb)
+nf_bridge_get_physindev(const struct sk_buff *skb, struct net *net)
(all callers have a struct net available).
No need to rename the function, see below.
> - br_indev = nf_bridge_get_physindev(oldskb);
> + rcu_read_lock_bh();
> + br_indev = nf_bridge_get_physindev_rcu(oldskb);
No need for rcu read lock, all netfilter hooks run inside
rcu_read_lock().
> Does it sound good?
Yes, seems ok to me.
> Or maybe instead we can have extra physindev_if field in addition to
> existing physindev to only do dev_get_by_index_rcu inside
> br_nf_pre_routing_finish_bridge_slow to doublecheck the ->physindev link?
>
> Sorry in advance if I'm missing anything obvious.
Alternative would be to add a 'br_nf_unreg_serno' that gets incremented
from brnf_device_event(), then store that in nf_bridge_info struct and
compare to current value before net_device deref. If not equal, toss skb.
Problem is that we'd need some indirection to retrieve the current
value, otherwise places like nfnetlink_log() gain a module dependency on
br_netfilter :-(
We'd likely need
const atomic_t *br_nf_unreg_serno __read_mostly;
EXPORT_SYMBOL_GPL(br_nf_unreg_serno);
in net/netfilter/core.c for this, then set/clear the
pointer from br_netfilter_hooks.c.
I can't say/don't know which of the two options is better/worse.
s/struct net_device */int// has the benefit of shrinking nf_bridge_info,
so I'd try that first.
Here is the new version
https://lore.kernel.org/netdev/[email protected]/
On 09/01/2024 19:12, Florian Westphal wrote:
> Pavel Tikhomirov <[email protected]> wrote:
>> index f980edfdd2783..105fbdb029261 100644
>> --- a/include/linux/netfilter_bridge.h
>> +++ b/include/linux/netfilter_bridge.h
>> @@ -56,11 +56,15 @@ static inline int nf_bridge_get_physoutif(const struct
>> sk_buff *skb)
>> }
>>
>> static inline struct net_device *
>> -nf_bridge_get_physindev(const struct sk_buff *skb)
>> +nf_bridge_get_physindev_rcu(const struct sk_buff *skb)
>> {
>> const struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
>> + struct net_device *dev;
>>
>> - return nf_bridge ? nf_bridge->physindev : NULL;
>> + if (!nf_bridge || !skb->dev)
>> + return 0;
>> +
>> + return dev_get_by_index_rcu(skb->dev->net, nf_bridge->physindev_if);
>
> You could use dev_net(skb->dev), yes.
In br_nf_pre_routing_finish_bridge_slow I had to use dev_net(skb->dev).
>
> Or create a preparation patch that does:
>
> -nf_bridge_get_physindev(const struct sk_buff *skb)
> +nf_bridge_get_physindev(const struct sk_buff *skb, struct net *net)
>
> (all callers have a struct net available).
For all other cases I did the prep-patch propagating net.
>
> No need to rename the function, see below.
>
>> - br_indev = nf_bridge_get_physindev(oldskb);
>> + rcu_read_lock_bh();
>> + br_indev = nf_bridge_get_physindev_rcu(oldskb);
>
> No need for rcu read lock, all netfilter hooks run inside
> rcu_read_lock().
Thanks for this hint! I have checked all those tons of cases and
actually proved to myself that all cases have rcu_read_lock =)
>
>> Does it sound good?
>
> Yes, seems ok to me.
>
>> Or maybe instead we can have extra physindev_if field in addition to
>> existing physindev to only do dev_get_by_index_rcu inside
>> br_nf_pre_routing_finish_bridge_slow to doublecheck the ->physindev link?
>>
>> Sorry in advance if I'm missing anything obvious.
>
> Alternative would be to add a 'br_nf_unreg_serno' that gets incremented
> from brnf_device_event(), then store that in nf_bridge_info struct and
> compare to current value before net_device deref. If not equal, toss skb.
>
> Problem is that we'd need some indirection to retrieve the current
> value, otherwise places like nfnetlink_log() gain a module dependency on
> br_netfilter :-(
>
> We'd likely need
> const atomic_t *br_nf_unreg_serno __read_mostly;
> EXPORT_SYMBOL_GPL(br_nf_unreg_serno);
>
> in net/netfilter/core.c for this, then set/clear the
> pointer from br_netfilter_hooks.c.
>
> I can't say/don't know which of the two options is better/worse.
>
> s/struct net_device */int// has the benefit of shrinking nf_bridge_info,
> so I'd try that first.
Ok, did s/struct net_device */int// variant.
--
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.