netdev: Use flexible array for trailing private bytes

PATCH In review Kees Cook 7 participants 14 messages Feb 29, 2024 – Mar 6, 2024

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Kees Cook

author

2024-02-29 21:30 UTC #0 lore raw

PATCH 1/1 netdev: Use flexible array for trailing private bytes

Introduce a new struct net_device_priv that contains struct net_device but also accounts for the commonly trailing bytes through the "size" and "data" members. As many dummy struct net_device instances exist still, it is non-trivial to but this flexible array inside struct net_device itself. But we can add a sanity check in netdev_priv() to catch any attempts to access the private data of a dummy struct.

Adjust allocation logic to use the new full structure.

Signed-off-by Kees Cook <[email protected]>

---
Cc: Jakub Kicinski <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: "Gustavo A. R. Silva" <[email protected]>
Cc: [email protected]
Cc: [email protected]

---
 include/linux/netdevice.h | 21 ++++++++++++++++++---
 net/core/dev.c            | 12 ++++--------
 2 files changed, 22 insertions(+), 11 deletions(-)

include/linux/netdevice.h +18 -3

index 118c40258d07..b476809d0bae 100644
@@ -1815,6 +1815,8 @@ enum netdev_stat_type {
 	NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
 };
 
+#define	NETDEV_ALIGN		32
+
 /**
  *	struct net_device - The DEVICE structure.
  *

@@ -2476,6 +2478,14 @@ struct net_device {
 	struct hlist_head	page_pools;
 #endif
 };
+
+struct net_device_priv {
+	struct net_device	dev;
+	u32			size;
+	u8			data[] __counted_by(size)
+				       __aligned(NETDEV_ALIGN);
+};
+
 #define to_net_dev(d) container_of(d, struct net_device, dev)
 
 /*

@@ -2496,8 +2506,6 @@ static inline bool netif_elide_gro(const struct net_device *dev)
 	return false;
 }
 
-#define	NETDEV_ALIGN		32
-
 static inline
 int netdev_get_prio_tc_map(const struct net_device *dev, u32 prio)
 {

@@ -2665,7 +2673,14 @@ void dev_net_set(struct net_device *dev, struct net *net)
  */
 static inline void *netdev_priv(const struct net_device *dev)
 {
-	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
+	struct net_device_priv *priv;
+
+	/* Dummy struct net_device have no trailing data. */
+	if (WARN_ON_ONCE(dev->reg_state == NETREG_DUMMY))
+		return NULL;
+
+	priv = container_of(dev, struct net_device_priv, dev);
+	return (u8 *)priv->data;
 }
 
 /* Set the sysfs physical device reference for the network logical device

net/core/dev.c +4 -8

index cb2dab0feee0..0fcaf6ae8486 100644
@@ -10800,7 +10800,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 {
 	struct net_device *dev;
 	unsigned int alloc_size;
-	struct net_device *p;
+	struct net_device_priv *p;
 
 	BUG_ON(strlen(name) >= sizeof(dev->name));

@@ -10814,20 +10814,16 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 		return NULL;
 	}
 
-	alloc_size = sizeof(struct net_device);
-	if (sizeof_priv) {
-		/* ensure 32-byte alignment of private area */
-		alloc_size = ALIGN(alloc_size, NETDEV_ALIGN);
-		alloc_size += sizeof_priv;
-	}
+	alloc_size = struct_size(p, data, sizeof_priv);
 	/* ensure 32-byte alignment of whole construct */
 	alloc_size += NETDEV_ALIGN - 1;
 
 	p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL);
 	if (!p)
 		return NULL;
+	p->size = sizeof_priv;
 
-	dev = PTR_ALIGN(p, NETDEV_ALIGN);
+	dev = &PTR_ALIGN(p, NETDEV_ALIGN)->dev;
 	dev->padded = (char *)dev - (char *)p;
 
 	ref_tracker_dir_init(&dev->refcnt_tracker, 128, name);

Signature · 1 line

2.34.1

Gustavo A. R. Silva

Reviewed-by on v1 1/1

2024-02-29 22:15 UTC #1 lore raw

Quoted 10 lines · Kees Cook

Introduce a new struct net_device_priv that contains struct net_device but also accounts for the commonly trailing bytes through the "size" and "data" members. As many dummy struct net_device instances exist still, it is non-trivial to but this flexible array inside struct net_device itself. But we can add a sanity check in netdev_priv() to catch any attempts to access the private data of a dummy struct.
Adjust allocation logic to use the new full structure.
Signed-off-by Kees Cook <[email protected]>

Reviewed-by Gustavo A. R. Silva <[email protected]>

[for the flex struct net_device_priv, struct_size(), __counted_by(), and the use of container_of() to retrieve a pointer to the flex struct and return pointer to flex-array member data in netdev_priv()]

Thanks
--
Gustavo

Quoted 104 lines

---
  include/linux/netdevice.h | 21 ++++++++++++++++++---
  net/core/dev.c            | 12 ++++--------
  2 files changed, 22 insertions(+), 11 deletions(-)

include/linux/netdevice.h +18 -3

index 118c40258d07..b476809d0bae 100644
@@ -1815,6 +1815,8 @@ enum netdev_stat_type {
  	NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
  };
 
+#define	NETDEV_ALIGN		32
+
  /**
   *	struct net_device - The DEVICE structure.
   *

@@ -2476,6 +2478,14 @@ struct net_device {
  	struct hlist_head	page_pools;
  #endif
  };
+
+struct net_device_priv {
+	struct net_device	dev;
+	u32			size;
+	u8			data[] __counted_by(size)
+				       __aligned(NETDEV_ALIGN);
+};
+
  #define to_net_dev(d) container_of(d, struct net_device, dev)
 
  /*

@@ -2496,8 +2506,6 @@ static inline bool netif_elide_gro(const struct net_device *dev)
  	return false;
  }
 
-#define	NETDEV_ALIGN		32
-
  static inline
  int netdev_get_prio_tc_map(const struct net_device *dev, u32 prio)
  {

@@ -2665,7 +2673,14 @@ void dev_net_set(struct net_device *dev, struct net *net)
   */
  static inline void *netdev_priv(const struct net_device *dev)
  {
-	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
+	struct net_device_priv *priv;
+
+	/* Dummy struct net_device have no trailing data. */
+	if (WARN_ON_ONCE(dev->reg_state == NETREG_DUMMY))
+		return NULL;
+
+	priv = container_of(dev, struct net_device_priv, dev);
+	return (u8 *)priv->data;
  }
 
  /* Set the sysfs physical device reference for the network logical device

net/core/dev.c +4 -8

index cb2dab0feee0..0fcaf6ae8486 100644
@@ -10800,7 +10800,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
  {
  	struct net_device *dev;
  	unsigned int alloc_size;
-	struct net_device *p;
+	struct net_device_priv *p;
 
  	BUG_ON(strlen(name) >= sizeof(dev->name));

@@ -10814,20 +10814,16 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
  		return NULL;
  	}
 
-	alloc_size = sizeof(struct net_device);
-	if (sizeof_priv) {
-		/* ensure 32-byte alignment of private area */
-		alloc_size = ALIGN(alloc_size, NETDEV_ALIGN);
-		alloc_size += sizeof_priv;
-	}
+	alloc_size = struct_size(p, data, sizeof_priv);
  	/* ensure 32-byte alignment of whole construct */
  	alloc_size += NETDEV_ALIGN - 1;
 
  	p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL);
  	if (!p)
  		return NULL;
+	p->size = sizeof_priv;
 
-	dev = PTR_ALIGN(p, NETDEV_ALIGN);
+	dev = &PTR_ALIGN(p, NETDEV_ALIGN)->dev;
  	dev->padded = (char *)dev - (char *)p;
 
  	ref_tracker_dir_init(&dev->refcnt_tracker, 128, name);

Jakub Kicinski

maintainer

2024-03-01 06:59 UTC #2 lore raw

In reply to Kees Cook

Quoted 3 lines · On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:

Introduce a new struct net_device_priv that contains struct net_device but also accounts for the commonly trailing bytes through the "size" and "data" members.

I'm a bit unclear on the benefit. Perhaps I'm unaccustomed to "safe C".

Quoted 2 lines

As many dummy struct net_device instances exist still,
it is non-trivial to but this flexible array inside struct net_device

put

Non-trivial, meaning what's the challenge?
We also do somewhat silly things with netdev lifetime, because we can't assume netdev gets freed by netdev_free(). Cleaning up the "embedders" would be beneficial for multiple reasons.

Quoted 12 lines

itself. But we can add a sanity check in netdev_priv() to catch any attempts to access the private data of a dummy struct.
Adjust allocation logic to use the new full structure.
Signed-off-by Kees Cook <[email protected]>
include/linux/netdevice.h +0 -0
index 118c40258d07..b476809d0bae 100644

include/linux/netdevice.h enum netdev_stat_type { · line 1818

Quoted 4 lines

include/linux/netdevice.h +1 -0

@@ -1815,6 +1815,8 @@ enum netdev_stat_type {
 	NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
 };
 
+#define	NETDEV_ALIGN		32

Unless someone knows what this is for it should go. Align priv to cacheline size.

Quoted 5 lines

include/linux/netdevice.h +0 -0

 /**
  *	struct net_device - The DEVICE structure.
  *

include/linux/netdevice.h void dev_net_set(struct net_device *dev, struct net *net) · line 2680

Quoted 9 lines

include/linux/netdevice.h +5 -1

@@ -2665,7 +2673,14 @@ void dev_net_set(struct net_device *dev, struct net *net)
  */
 static inline void *netdev_priv(const struct net_device *dev)
 {
-	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
+	struct net_device_priv *priv;
+
+	/* Dummy struct net_device have no trailing data. */
+	if (WARN_ON_ONCE(dev->reg_state == NETREG_DUMMY))
+		return NULL;

This is a static inline with roughly 11,000 call sites, according to a quick grep. Aren't WARN_ONCE() in static inlines creating a "once" object in every compilation unit where they get used?

Quoted 3 lines

include/linux/netdevice.h +2 -0

+	priv = container_of(dev, struct net_device_priv, dev);
+	return (u8 *)priv->data;
 }

Eric Dumazet

2024-03-01 08:04 UTC #3 lore raw

Quoted 11 lines · Jakub Kicinski

Quoted 8 lines · On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
include/linux/netdevice.h +0 -0
index 118c40258d07..b476809d0bae 100644
@@ -1815,6 +1815,8 @@ enum netdev_stat_type {
      NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
 };
 

Quoted 4 lines

include/linux/netdevice.h enum netdev_stat_type { · line 1818
Quoted 1 line
include/linux/netdevice.h +1 -0
+#define      NETDEV_ALIGN            32
Unless someone knows what this is for it should go. Align priv to cacheline size.

#define NETDEV_ALIGN L1_CACHE_BYTES

or a general replacement of NETDEV_ALIGN....

Alexander Lobakin

2024-03-01 12:59 UTC #4 lore raw

From: Eric Dumazet <[email protected]>
Date: Fri, 1 Mar 2024 09:03:55 +0100

Quoted 3 lines · Jakub Kicinski

Quoted 2 lines · On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:

Re WARN_ONCE() in netdev_priv(): netdev_priv() is VERY hot, I'm not sure we want to add checks there. Maybe under CONFIG_DEBUG_NET?

Quoted 9 lines

include/linux/netdevice.h +0 -0

index 118c40258d07..b476809d0bae 100644
@@ -1815,6 +1815,8 @@ enum netdev_stat_type {
      NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
 };

Quoted 7 lines

Quoted 4 lines
include/linux/netdevice.h enum netdev_stat_type { · line 1818
Quoted 1 line
include/linux/netdevice.h +1 -0
+#define      NETDEV_ALIGN            32
Unless someone knows what this is for it should go. Align priv to cacheline size.
+2

Maybe

Quoted 1 line

#define NETDEV_ALIGN L1_CACHE_BYTES

#define NETDEV_ALIGN max(SMP_CACHE_BYTES, 32)

(or even max(1 << INTERNODE_CACHE_SHIFT, 32))

Quoted 4 lines

or a general replacement of NETDEV_ALIGN....

+ I'd align both struct net_device AND its private space to %NETDEV_ALIGN and remove this weird PTR_ALIGN. {k,v}malloc ensures natural alignment of allocations for at least a couple years already (IOW if struct net_device is aligned to 64, {k,v}malloc will *always* return a 64-byte aligned address).

Thanks,
Olek

Eric Dumazet

2024-03-01 13:25 UTC #5 lore raw

Quoted 25 lines · Alexander Lobakin

From: Eric Dumazet <[email protected]>
Date: Fri, 1 Mar 2024 09:03:55 +0100
Quoted 3 lines · Jakub Kicinski
Quoted 2 lines · On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
Re WARN_ONCE() in netdev_priv(): netdev_priv() is VERY hot, I'm not sure we want to add checks there. Maybe under CONFIG_DEBUG_NET?
Quoted 14 lines
Quoted 13 lines
include/linux/netdevice.h enum netdev_stat_type { · line 1818
Quoted 10 lines
include/linux/netdevice.h +1 -0
index 118c40258d07..b476809d0bae 100644
@@ -1815,6 +1815,8 @@ enum netdev_stat_type {
      NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
 };
 
+#define      NETDEV_ALIGN            32
Unless someone knows what this is for it should go. Align priv to cacheline size.

Quoted 8 lines

Quoted 2 lines
+2
Maybe
Quoted 1 line
#define NETDEV_ALIGN L1_CACHE_BYTES
#define NETDEV_ALIGN max(SMP_CACHE_BYTES, 32)

Why would we care if some arches have a very small SMP_CACHE_BYTES ? Bet it !

IMO nothing in networking mandates this minimal 32 byte alignment.

Quoted 4 lines

?
(or even max(1 << INTERNODE_CACHE_SHIFT, 32))

I do not think so.

INTERNODE_CACHE_SHIFT is a bit extreme on allyesconfig on x86 :/ (with CONFIG_X86_VSMP=y)

Quoted 11 lines

Quoted 5 lines
or a general replacement of NETDEV_ALIGN....
+ I'd align both struct net_device AND its private space to %NETDEV_ALIGN and remove this weird PTR_ALIGN. {k,v}malloc ensures natural alignment of allocations for at least a couple years already (IOW if struct net_device is aligned to 64, {k,v}malloc will *always* return a 64-byte aligned address).

I think that with SLAB or SLOB in the past with some DEBUG options there was no such guarantee.

But this is probably no longer the case, and heavy DEBUG options these days (KASAN, KFENCE...)
do not expect fast networking anyway.

Alexander Lobakin

2024-03-01 14:30 UTC #6 lore raw

From: Eric Dumazet <[email protected]>
Date: Fri, 1 Mar 2024 14:25:37 +0100

Quoted 30 lines

Quoted 28 lines · Alexander Lobakin
From: Eric Dumazet <[email protected]>
Date: Fri, 1 Mar 2024 09:03:55 +0100
Quoted 3 lines · Jakub Kicinski
Quoted 2 lines · On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
Re WARN_ONCE() in netdev_priv(): netdev_priv() is VERY hot, I'm not sure we want to add checks there. Maybe under CONFIG_DEBUG_NET?
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
include/linux/netdevice.h +0 -0
index 118c40258d07..b476809d0bae 100644
@@ -1815,6 +1815,8 @@ enum netdev_stat_type {
      NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
 };
 
Quoted 4 lines
include/linux/netdevice.h enum netdev_stat_type { · line 1818
Quoted 1 line
include/linux/netdevice.h +1 -0
+#define      NETDEV_ALIGN            32
Unless someone knows what this is for it should go. Align priv to cacheline size.
+2

Quoted 7 lines

Quoted 5 lines
Maybe
Quoted 1 line
#define NETDEV_ALIGN L1_CACHE_BYTES
#define NETDEV_ALIGN max(SMP_CACHE_BYTES, 32)
Why would we care if some arches have a very small SMP_CACHE_BYTES ?

Oh sorry, I thought %SMP_CACHE_BYTES is 1 when !SMP.
We can then just add ____cacheline_aligned to both struct net_device and its ::priv flex array and that's it.

I like the idea of declaring priv explicitly rather than doing size + ptr magic. But maybe we could just add this flex array to struct net_device and avoid introducing a new structure.

Quoted 27 lines

Bet it !
IMO nothing in networking mandates this minimal 32 byte alignment.
Quoted 4 lines
?
(or even max(1 << INTERNODE_CACHE_SHIFT, 32))
I do not think so.
INTERNODE_CACHE_SHIFT is a bit extreme on allyesconfig on x86 :/ (with CONFIG_X86_VSMP=y)
Quoted 11 lines
Quoted 5 lines
or a general replacement of NETDEV_ALIGN....
+ I'd align both struct net_device AND its private space to %NETDEV_ALIGN and remove this weird PTR_ALIGN. {k,v}malloc ensures natural alignment of allocations for at least a couple years already (IOW if struct net_device is aligned to 64, {k,v}malloc will *always* return a 64-byte aligned address).

Quoted 6 lines

I think that with SLAB or SLOB in the past with some DEBUG options there was no such guarantee.
But this is probably no longer the case, and heavy DEBUG options these days (KASAN, KFENCE...)
do not expect fast networking anyway.

Thanks,
Olek

Jakub Kicinski

maintainer

2024-03-01 17:35 UTC #7 lore raw

Quoted 3 lines · On Fri, 1 Mar 2024 15:30:03 +0100 Alexander Lobakin wrote:

I like the idea of declaring priv explicitly rather than doing size + ptr magic. But maybe we could just add this flex array to struct net_device and avoid introducing a new structure.

100% I should have linked to the thread that led to Kees's work. Adding directly to net_device would be way better but there's a handful of drivers which embed the struct.
If we can switch them to dynamic allocation, that'd be great. And, as you may be alluding to, it removes the need for the WARN_ON() entirely as well.

Alexander Lobakin

2024-03-04 14:35 UTC #8 lore raw

From: Jakub Kicinski <[email protected]>
Date: Fri, 1 Mar 2024 09:35:17 -0800

Quoted 8 lines · On Fri, 1 Mar 2024 15:30:03 +0100 Alexander Lobakin wrote:

Quoted 3 lines
I like the idea of declaring priv explicitly rather than doing size + ptr magic. But maybe we could just add this flex array to struct net_device and avoid introducing a new structure.
100% I should have linked to the thread that led to Kees's work. Adding directly to net_device would be way better but there's a handful of drivers which embed the struct.

I think it's okay to embed a struct with flex array at the end as long as it's not used? Or the compiler will say that the flex array is not at the end of the structure?

Quoted 1 line

If we can switch them to dynamic allocation, that'd be great.

It's mega weird to embed &net_device rather than do alloc_*dev() >_<

Quoted 2 lines

And, as you may be alluding to, it removes the need for the WARN_ON() entirely as well.

Thanks,
Olek

Jakub Kicinski

maintainer

2024-03-04 15:24 UTC #9 lore raw

Quoted 7 lines · On Mon, 4 Mar 2024 15:32:51 +0100 Alexander Lobakin wrote:

Quoted 3 lines
100% I should have linked to the thread that led to Kees's work. Adding directly to net_device would be way better but there's a handful of drivers which embed the struct.
I think it's okay to embed a struct with flex array at the end as long as it's not used? Or the compiler will say that the flex array is not at the end of the structure?

Technically, yes. Practically it ties the lifetime of a refcounted object to something semi-related with different lifetime rules :(

Greg KH

2024-03-01 11:41 UTC #10 lore raw

In reply to Jakub Kicinski

Quoted 41 lines · Jakub Kicinski

Quoted 3 lines · On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
Introduce a new struct net_device_priv that contains struct net_device but also accounts for the commonly trailing bytes through the "size" and "data" members.
I'm a bit unclear on the benefit. Perhaps I'm unaccustomed to "safe C".
Quoted 2 lines
As many dummy struct net_device instances exist still,
it is non-trivial to but this flexible array inside struct net_device
put
Non-trivial, meaning what's the challenge?
We also do somewhat silly things with netdev lifetime, because we can't assume netdev gets freed by netdev_free(). Cleaning up the "embedders" would be beneficial for multiple reasons.
Quoted 12 lines
itself. But we can add a sanity check in netdev_priv() to catch any attempts to access the private data of a dummy struct.
Adjust allocation logic to use the new full structure.
Signed-off-by Kees Cook <[email protected]>
include/linux/netdevice.h +0 -0
index 118c40258d07..b476809d0bae 100644
include/linux/netdevice.h enum netdev_stat_type { · line 1818
Quoted 4 lines
include/linux/netdevice.h +1 -0
@@ -1815,6 +1815,8 @@ enum netdev_stat_type {
 	NETDEV_PCPU_STAT_DSTATS, /* struct pcpu_dstats */
 };
 
+#define	NETDEV_ALIGN		32
Unless someone knows what this is for it should go. Align priv to cacheline size.
Quoted 3 lines
include/linux/netdevice.h +0 -0
 /**
  *	struct net_device - The DEVICE structure.
  *

Quoted 14 lines

include/linux/netdevice.h void dev_net_set(struct net_device *dev, struct net *net) · line 2680
Quoted 10 lines
include/linux/netdevice.h +5 -1
@@ -2665,7 +2673,14 @@ void dev_net_set(struct net_device *dev, struct net *net)
  */
 static inline void *netdev_priv(const struct net_device *dev)
 {
-	return (char *)dev + ALIGN(sizeof(struct net_device), NETDEV_ALIGN);
+	struct net_device_priv *priv;
+
+	/* Dummy struct net_device have no trailing data. */
+	if (WARN_ON_ONCE(dev->reg_state == NETREG_DUMMY))
+		return NULL;
This is a static inline with roughly 11,000 call sites, according to a quick grep. Aren't WARN_ONCE() in static inlines creating a "once" object in every compilation unit where they get used?

It also, if this every trips, will reboot the box for those that run with panic-on-warn set, is that something that you all really want?

thanks,

greg k-h

Breno Leitao

2024-03-06 13:16 UTC #11 lore raw

In reply to Jakub Kicinski

Quoted 10 lines · Jakub Kicinski

Quoted 3 lines · On Thu, 29 Feb 2024 13:30:22 -0800 Kees Cook wrote:
Introduce a new struct net_device_priv that contains struct net_device but also accounts for the commonly trailing bytes through the "size" and "data" members.
I'm a bit unclear on the benefit. Perhaps I'm unaccustomed to "safe C".
Quoted 2 lines
As many dummy struct net_device instances exist still,
it is non-trivial to but this flexible array inside struct net_device

Quoted 6 lines

put
Non-trivial, meaning what's the challenge?
We also do somewhat silly things with netdev lifetime, because we can't assume netdev gets freed by netdev_free(). Cleaning up the "embedders" would be beneficial for multiple reasons.

I've been looking at some of these embedders as reported by Kees[1], and most of them are for dummy interfaces. I.e, they are basically used for schedule NAPI poll.

From that list[1], most of the driver matches with:

	# git grep init_dummy_netdev

That said, do you think it is still worth cleaning up embedders for dummy net_devices?

[1] https://lore.kernel.org/all/202402281554.C1CEEF744@keescook/

Jakub Kicinski

maintainer

2024-03-06 15:06 UTC #12 lore raw

Quoted 4 lines · On Wed, 6 Mar 2024 05:16:16 -0800 Breno Leitao wrote:

I've been looking at some of these embedders as reported by Kees[1], and most of them are for dummy interfaces. I.e, they are basically used for schedule NAPI poll.

Quoted 8 lines

From that list[1], most of the driver matches with:
	# git grep init_dummy_netdev
That said, do you think it is still worth cleaning up embedders for dummy net_devices?
[1] https://lore.kernel.org/all/202402281554.C1CEEF744@keescook/

Yes, I think so.
Kees, did you plan to send a v2? Otherwise I can put the cleanup on our "public ToDo" list :)

Kees Cook

author

2024-03-06 23:42 UTC #13 lore raw

Quoted 9 lines · Jakub Kicinski

Quoted 8 lines · On Wed, 6 Mar 2024 05:16:16 -0800 Breno Leitao wrote:
I've been looking at some of these embedders as reported by Kees[1], and most of them are for dummy interfaces. I.e, they are basically used for schedule NAPI poll.
From that list[1], most of the driver matches with:
	# git grep init_dummy_netdev

Quoted 8 lines

Quoted 4 lines
That said, do you think it is still worth cleaning up embedders for dummy net_devices?
[1] https://lore.kernel.org/all/202402281554.C1CEEF744@keescook/
Yes, I think so.
Kees, did you plan to send a v2? Otherwise I can put the cleanup on our "public ToDo" list :)

I found the requested collateral changes that popped out of v1 to be rather a bit much for me to tackle right now, so I think adding to the TODO list is probably best. :)

-Kees

Signature · 1 line

Kees Cook