[PATCH net-next v3 05/10] genetlink: use attrs from struct genl_info

2023-08-14 Thread Jakub Kicinski 
Since dumps carry struct genl_info now, use the attrs pointer
from genl_info and remove the one in struct genl_dumpit_info.

Reviewed-by: Johannes Berg 
Reviewed-by: Miquel Raynal 
Reviewed-by: Jiri Pirko 
Signed-off-by: Jakub Kicinski 
---
CC: ja...@zx2c4.com
CC: j...@resnulli.us
CC: alex.ar...@gmail.com
CC: ste...@datenfreihafen.org
CC: krzysztof.kozlow...@linaro.org
CC: jma...@redhat.com
CC: ying@windriver.com
CC: floridslee...@gmail.com
CC: l...@kernel.org
CC: jacob.e.kel...@intel.com
CC: wireguard@lists.zx2c4.com
CC: linux-w...@vger.kernel.org
CC: tipc-discuss...@lists.sourceforge.net
---
 drivers/net/wireguard/netlink.c | 2 +-
 include/net/genetlink.h | 1 -
 net/devlink/health.c| 2 +-
 net/devlink/leftover.c  | 6 +++---
 net/ethtool/netlink.c   | 3 ++-
 net/ethtool/tunnels.c   | 2 +-
 net/ieee802154/nl802154.c   | 4 ++--
 net/netlink/genetlink.c | 7 +++
 net/nfc/netlink.c   | 4 ++--
 net/tipc/netlink_compat.c   | 2 +-
 net/tipc/node.c | 4 ++--
 net/tipc/socket.c   | 2 +-
 net/tipc/udp_media.c| 2 +-
 13 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
index 6d1bd9f52d02..dc09b75a3248 100644
--- a/drivers/net/wireguard/netlink.c
+++ b/drivers/net/wireguard/netlink.c
@@ -200,7 +200,7 @@ static int wg_get_device_start(struct netlink_callback *cb)
 {
struct wg_device *wg;
 
-   wg = lookup_interface(genl_dumpit_info(cb)->attrs, cb->skb);
+   wg = lookup_interface(genl_info_dump(cb)->attrs, cb->skb);
if (IS_ERR(wg))
return PTR_ERR(wg);
DUMP_CTX(cb)->wg = wg;
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 86c8eaaa3a43..a8a15b9c22c8 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -255,7 +255,6 @@ struct genl_split_ops {
 struct genl_dumpit_info {
const struct genl_family *family;
struct genl_split_ops op;
-   struct nlattr **attrs;
struct genl_info info;
 };
 
diff --git a/net/devlink/health.c b/net/devlink/health.c
index a85bdec34801..59e7cff22d97 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -1266,7 +1266,7 @@ devlink_health_reporter_get_from_cb(struct 
netlink_callback *cb)
 {
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct devlink_health_reporter *reporter;
-   struct nlattr **attrs = info->attrs;
+   struct nlattr **attrs = info->info.attrs;
struct devlink *devlink;
 
devlink = devlink_get_from_attrs_lock(sock_net(cb->skb->sk), attrs);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 3883a90d32bb..72ba8a716525 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -5201,7 +5201,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct 
sk_buff *skb,
struct devlink_nl_dump_state *state = devlink_dump_state(cb);
struct nlattr *chunks_attr, *region_attr, *snapshot_attr;
u64 ret_offset, start_offset, end_offset = U64_MAX;
-   struct nlattr **attrs = info->attrs;
+   struct nlattr **attrs = info->info.attrs;
struct devlink_port *port = NULL;
devlink_chunk_fill_t *region_cb;
struct devlink_region *region;
@@ -5224,8 +5224,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct 
sk_buff *skb,
goto out_unlock;
}
 
-   if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
-   index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+   if (attrs[DEVLINK_ATTR_PORT_INDEX]) {
+   index = nla_get_u32(attrs[DEVLINK_ATTR_PORT_INDEX]);
 
port = devlink_port_get_by_index(devlink, index);
if (!port) {
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index ae344f1b0bbd..9fc7c41f4786 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -538,7 +538,8 @@ static int ethnl_default_start(struct netlink_callback *cb)
goto free_req_info;
}
 
-   ret = ethnl_default_parse(req_info, info->attrs, sock_net(cb->skb->sk),
+   ret = ethnl_default_parse(req_info, info->info.attrs,
+ sock_net(cb->skb->sk),
  ops, cb->extack, false);
if (req_info->dev) {
/* We ignore device specification in dump requests but as the
diff --git a/net/ethtool/tunnels.c b/net/ethtool/tunnels.c
index 05f752557b5e..b4ce47dd2aa6 100644
--- a/net/ethtool/tunnels.c
+++ b/net/ethtool/tunnels.c
@@ -219,7 +219,7 @@ int ethnl_tunnel_info_start(struct netlink_callback *cb)
 {
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct ethnl_tunnel_info_dump_ctx *ctx = (void *)cb->ctx;
-   struct nlattr **tb = info->attrs;
+   struct nlattr **tb = info->info.attrs;

[PATCH net-next v2 05/10] genetlink: use attrs from struct genl_info

2023-08-10 Thread Jakub Kicinski 
Since dumps carry struct genl_info now, use the attrs pointer
from genl_info and remove the one in struct genl_dumpit_info.

Reviewed-by: Johannes Berg 
Reviewed-by: Miquel Raynal 
Reviewed-by: Jiri Pirko 
Signed-off-by: Jakub Kicinski 
---
CC: ja...@zx2c4.com
CC: j...@resnulli.us
CC: alex.ar...@gmail.com
CC: ste...@datenfreihafen.org
CC: krzysztof.kozlow...@linaro.org
CC: jma...@redhat.com
CC: ying@windriver.com
CC: floridslee...@gmail.com
CC: l...@kernel.org
CC: jacob.e.kel...@intel.com
CC: wireguard@lists.zx2c4.com
CC: linux-w...@vger.kernel.org
CC: tipc-discuss...@lists.sourceforge.net
---
 drivers/net/wireguard/netlink.c | 2 +-
 include/net/genetlink.h | 1 -
 net/devlink/health.c| 2 +-
 net/devlink/leftover.c  | 6 +++---
 net/ethtool/netlink.c   | 3 ++-
 net/ethtool/tunnels.c   | 2 +-
 net/ieee802154/nl802154.c   | 4 ++--
 net/netlink/genetlink.c | 7 +++
 net/nfc/netlink.c   | 4 ++--
 net/tipc/netlink_compat.c   | 2 +-
 net/tipc/node.c | 4 ++--
 net/tipc/socket.c   | 2 +-
 net/tipc/udp_media.c| 2 +-
 13 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
index 6d1bd9f52d02..dc09b75a3248 100644
--- a/drivers/net/wireguard/netlink.c
+++ b/drivers/net/wireguard/netlink.c
@@ -200,7 +200,7 @@ static int wg_get_device_start(struct netlink_callback *cb)
 {
struct wg_device *wg;
 
-   wg = lookup_interface(genl_dumpit_info(cb)->attrs, cb->skb);
+   wg = lookup_interface(genl_info_dump(cb)->attrs, cb->skb);
if (IS_ERR(wg))
return PTR_ERR(wg);
DUMP_CTX(cb)->wg = wg;
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 86c8eaaa3a43..a8a15b9c22c8 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -255,7 +255,6 @@ struct genl_split_ops {
 struct genl_dumpit_info {
const struct genl_family *family;
struct genl_split_ops op;
-   struct nlattr **attrs;
struct genl_info info;
 };
 
diff --git a/net/devlink/health.c b/net/devlink/health.c
index 194340a8bb86..b8b3c09eea9e 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -1250,7 +1250,7 @@ devlink_health_reporter_get_from_cb(struct 
netlink_callback *cb)
 {
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct devlink_health_reporter *reporter;
-   struct nlattr **attrs = info->attrs;
+   struct nlattr **attrs = info->info.attrs;
struct devlink *devlink;
 
devlink = devlink_get_from_attrs_lock(sock_net(cb->skb->sk), attrs);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index e7900d9fa205..39538fb61008 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -5172,7 +5172,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct 
sk_buff *skb,
struct devlink_nl_dump_state *state = devlink_dump_state(cb);
struct nlattr *chunks_attr, *region_attr, *snapshot_attr;
u64 ret_offset, start_offset, end_offset = U64_MAX;
-   struct nlattr **attrs = info->attrs;
+   struct nlattr **attrs = info->info.attrs;
struct devlink_port *port = NULL;
devlink_chunk_fill_t *region_cb;
struct devlink_region *region;
@@ -5195,8 +5195,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct 
sk_buff *skb,
goto out_unlock;
}
 
-   if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
-   index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+   if (attrs[DEVLINK_ATTR_PORT_INDEX]) {
+   index = nla_get_u32(attrs[DEVLINK_ATTR_PORT_INDEX]);
 
port = devlink_port_get_by_index(devlink, index);
if (!port) {
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index ae344f1b0bbd..9fc7c41f4786 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -538,7 +538,8 @@ static int ethnl_default_start(struct netlink_callback *cb)
goto free_req_info;
}
 
-   ret = ethnl_default_parse(req_info, info->attrs, sock_net(cb->skb->sk),
+   ret = ethnl_default_parse(req_info, info->info.attrs,
+ sock_net(cb->skb->sk),
  ops, cb->extack, false);
if (req_info->dev) {
/* We ignore device specification in dump requests but as the
diff --git a/net/ethtool/tunnels.c b/net/ethtool/tunnels.c
index 05f752557b5e..b4ce47dd2aa6 100644
--- a/net/ethtool/tunnels.c
+++ b/net/ethtool/tunnels.c
@@ -219,7 +219,7 @@ int ethnl_tunnel_info_start(struct netlink_callback *cb)
 {
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct ethnl_tunnel_info_dump_ctx *ctx = (void *)cb->ctx;
-   struct nlattr **tb = info->attrs;
+   struct nlattr **tb = info->info.attrs;

[PATCH net-next 05/10] genetlink: use attrs from struct genl_info

2023-08-09 Thread Jakub Kicinski 
Since dumps carry struct genl_info now, use the attrs pointer
use the attr pointer from genl_info and remove the one in
struct genl_dumpit_info.

Signed-off-by: Jakub Kicinski 
---
CC: ja...@zx2c4.com
CC: j...@resnulli.us
CC: alex.ar...@gmail.com
CC: ste...@datenfreihafen.org
CC: miquel.ray...@bootlin.com
CC: krzysztof.kozlow...@linaro.org
CC: jma...@redhat.com
CC: ying@windriver.com
CC: floridslee...@gmail.com
CC: l...@kernel.org
CC: jacob.e.kel...@intel.com
CC: wireguard@lists.zx2c4.com
CC: linux-w...@vger.kernel.org
CC: tipc-discuss...@lists.sourceforge.net
---
 drivers/net/wireguard/netlink.c | 2 +-
 include/net/genetlink.h | 1 -
 net/devlink/health.c| 2 +-
 net/devlink/leftover.c  | 6 +++---
 net/ethtool/netlink.c   | 3 ++-
 net/ethtool/tunnels.c   | 2 +-
 net/ieee802154/nl802154.c   | 4 ++--
 net/netlink/genetlink.c | 7 +++
 net/nfc/netlink.c   | 4 ++--
 net/tipc/netlink_compat.c   | 2 +-
 net/tipc/node.c | 4 ++--
 net/tipc/socket.c   | 2 +-
 net/tipc/udp_media.c| 2 +-
 13 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireguard/netlink.c b/drivers/net/wireguard/netlink.c
index 6d1bd9f52d02..dc09b75a3248 100644
--- a/drivers/net/wireguard/netlink.c
+++ b/drivers/net/wireguard/netlink.c
@@ -200,7 +200,7 @@ static int wg_get_device_start(struct netlink_callback *cb)
 {
struct wg_device *wg;
 
-   wg = lookup_interface(genl_dumpit_info(cb)->attrs, cb->skb);
+   wg = lookup_interface(genl_info_dump(cb)->attrs, cb->skb);
if (IS_ERR(wg))
return PTR_ERR(wg);
DUMP_CTX(cb)->wg = wg;
diff --git a/include/net/genetlink.h b/include/net/genetlink.h
index 86c8eaaa3a43..a8a15b9c22c8 100644
--- a/include/net/genetlink.h
+++ b/include/net/genetlink.h
@@ -255,7 +255,6 @@ struct genl_split_ops {
 struct genl_dumpit_info {
const struct genl_family *family;
struct genl_split_ops op;
-   struct nlattr **attrs;
struct genl_info info;
 };
 
diff --git a/net/devlink/health.c b/net/devlink/health.c
index 194340a8bb86..b8b3c09eea9e 100644
--- a/net/devlink/health.c
+++ b/net/devlink/health.c
@@ -1250,7 +1250,7 @@ devlink_health_reporter_get_from_cb(struct 
netlink_callback *cb)
 {
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct devlink_health_reporter *reporter;
-   struct nlattr **attrs = info->attrs;
+   struct nlattr **attrs = info->info.attrs;
struct devlink *devlink;
 
devlink = devlink_get_from_attrs_lock(sock_net(cb->skb->sk), attrs);
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 3bf42f5335ed..98ee57a490e9 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -5172,7 +5172,7 @@ static int devlink_nl_cmd_region_read_dumpit(struct 
sk_buff *skb,
struct devlink_nl_dump_state *state = devlink_dump_state(cb);
struct nlattr *chunks_attr, *region_attr, *snapshot_attr;
u64 ret_offset, start_offset, end_offset = U64_MAX;
-   struct nlattr **attrs = info->attrs;
+   struct nlattr **attrs = info->info.attrs;
struct devlink_port *port = NULL;
devlink_chunk_fill_t *region_cb;
struct devlink_region *region;
@@ -5195,8 +5195,8 @@ static int devlink_nl_cmd_region_read_dumpit(struct 
sk_buff *skb,
goto out_unlock;
}
 
-   if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
-   index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+   if (attrs[DEVLINK_ATTR_PORT_INDEX]) {
+   index = nla_get_u32(attrs[DEVLINK_ATTR_PORT_INDEX]);
 
port = devlink_port_get_by_index(devlink, index);
if (!port) {
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index ae344f1b0bbd..9fc7c41f4786 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -538,7 +538,8 @@ static int ethnl_default_start(struct netlink_callback *cb)
goto free_req_info;
}
 
-   ret = ethnl_default_parse(req_info, info->attrs, sock_net(cb->skb->sk),
+   ret = ethnl_default_parse(req_info, info->info.attrs,
+ sock_net(cb->skb->sk),
  ops, cb->extack, false);
if (req_info->dev) {
/* We ignore device specification in dump requests but as the
diff --git a/net/ethtool/tunnels.c b/net/ethtool/tunnels.c
index 05f752557b5e..b4ce47dd2aa6 100644
--- a/net/ethtool/tunnels.c
+++ b/net/ethtool/tunnels.c
@@ -219,7 +219,7 @@ int ethnl_tunnel_info_start(struct netlink_callback *cb)
 {
const struct genl_dumpit_info *info = genl_dumpit_info(cb);
struct ethnl_tunnel_info_dump_ctx *ctx = (void *)cb->ctx;
-   struct nlattr **tb = info->attrs;
+   struct nlattr **tb = info->info.attrs;
int ret;
 
BUILD_BUG_ON

Re: [syzbot] [wireguard?] KASAN: slab-use-after-free Write in enqueue_timer

2023-05-24 Thread Jakub Kicinski 
On Wed, 24 May 2023 08:33:41 -0700 Jakub Kicinski wrote:
> On Wed, 24 May 2023 10:24:31 +0200 Dmitry Vyukov wrote:
> > FWIW There are more report examples on the dashboard.
> > There are some that don't mention wireguard nor usbnet, e.g.:
> > https://syzkaller.appspot.com/text?tag=CrashReport=17dd244628
> > So that's probably red herring. But they all seem to mention 
> > alloc_netdev_mqs.  
> 
> While we have you, let me ask about the possibility of having vmcore
> access - I think it'd be very useful to solve this mystery. 
> With a bit of luck the timer still has the function set.

I take that back.

Memory state around the buggy address:
 88801ecc1400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 88801ecc1480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>88801ecc1500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb  
   ^
 88801ecc1580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 88801ecc1600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb

Re: [syzbot] [wireguard?] KASAN: slab-use-after-free Write in enqueue_timer

2023-05-24 Thread Jakub Kicinski 
On Wed, 24 May 2023 10:24:31 +0200 Dmitry Vyukov wrote:
> FWIW There are more report examples on the dashboard.
> There are some that don't mention wireguard nor usbnet, e.g.:
> https://syzkaller.appspot.com/text?tag=CrashReport=17dd244628
> So that's probably red herring. But they all seem to mention alloc_netdev_mqs.

While we have you, let me ask about the possibility of having vmcore
access - I think it'd be very useful to solve this mystery. 
With a bit of luck the timer still has the function set.

Re: [syzbot] [wireguard?] KASAN: slab-use-after-free Write in enqueue_timer

2023-05-23 Thread Jakub Kicinski 
On Tue, 23 May 2023 18:42:53 +0200 Jason A. Donenfeld wrote:
> > It should, no idea why it isn't. Looking thru the code now I don't see
> > any obvious gaps where timer object is on a list but not active :S
> > There's no way to get a vmcore from syzbot, right? :)
> >
> > Also I thought the shutdown leads to a warning when someone tries to
> > schedule the dead timer but in fact add_timer() just exits cleanly.
> > So the shutdown won't help us find the culprit :(  
> 
> Worth noting that it could also be caused by adding to a dead timer
> anywhere in priv_data of another netdev, not just the sole timer_list
> in net_device.

Oh, I thought you zero'ed in on the watchdog based on offsets.
Still, object debug should track all timers in the slab and complain
on the free path.

Re: [syzbot] [wireguard?] KASAN: slab-use-after-free Write in enqueue_timer

2023-05-23 Thread Jakub Kicinski 
On Tue, 23 May 2023 18:14:18 +0200 Jason A. Donenfeld wrote:
> So, IOW, not a wireguard bug, right?

What's slightly concerning is that there aren't any other timers
leading to

  KASAN: slab-use-after-free Write in enqueue_timer

:( If WG was just an innocent bystander there should be, right?

Re: [syzbot] [wireguard?] KASAN: slab-use-after-free Write in enqueue_timer

2023-05-23 Thread Jakub Kicinski 
On Tue, 23 May 2023 18:12:32 +0200 Eric Dumazet wrote:
> > Your timer had the pleasure of getting queued _after_ a dead watchdog
> > timer, no? IOW it tries to update the ->next pointer of a queued
> > watchdog timer. We should probably do:
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 374d38fb8b9d..f3ed20ebcf5a 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -10389,6 +10389,8 @@ void netdev_run_todo(void)
> > WARN_ON(rcu_access_pointer(dev->ip_ptr));
> > WARN_ON(rcu_access_pointer(dev->ip6_ptr));
> >
> > +   WARN_ON(timer_shutdown_sync(>watchdog_timer));
> > +
> > if (dev->priv_destructor)
> > dev->priv_destructor(dev);
> > if (dev->needs_free_netdev)
> >
> > to catch how that watchdog_timer is getting queued. Would that make
> > sense, Eric?  
> 
> Would this case be catched at the time the device is freed ?
> 
> (CONFIG_DEBUG_OBJECTS_FREE=y or something)

It should, no idea why it isn't. Looking thru the code now I don't see
any obvious gaps where timer object is on a list but not active :S
There's no way to get a vmcore from syzbot, right? :)

Also I thought the shutdown leads to a warning when someone tries to
schedule the dead timer but in fact add_timer() just exits cleanly.
So the shutdown won't help us find the culprit :(

Re: [syzbot] [wireguard?] KASAN: slab-use-after-free Write in enqueue_timer

2023-05-23 Thread Jakub Kicinski 
On Tue, 23 May 2023 17:46:20 +0200 Jason A. Donenfeld wrote:
> > Freed by task 41:
> >  __kmem_cache_free+0x264/0x3c0 mm/slub.c:3799
> >  device_release+0x95/0x1c0
> >  kobject_cleanup lib/kobject.c:683 [inline]
> >  kobject_release lib/kobject.c:714 [inline]
> >  kref_put include/linux/kref.h:65 [inline]
> >  kobject_put+0x228/0x470 lib/kobject.c:731
> >  netdev_run_todo+0xe5a/0xf50 net/core/dev.c:10400  
> 
> So that means the memory in question is actually the one that's
> allocated and freed by the networking stack. Specifically, dev.c:10626
> is allocating a struct net_device with a trailing struct wg_device (its
> priv_data). However, wg_device does not have any struct timer_lists in
> it, and I don't see how net_device's watchdog_timer would be related to
> the stacktrace which is clearly operating over a wg_peer timer.
> 
> So what on earth is going on here?

Your timer had the pleasure of getting queued _after_ a dead watchdog
timer, no? IOW it tries to update the ->next pointer of a queued
watchdog timer. We should probably do:

diff --git a/net/core/dev.c b/net/core/dev.c
index 374d38fb8b9d..f3ed20ebcf5a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10389,6 +10389,8 @@ void netdev_run_todo(void)
WARN_ON(rcu_access_pointer(dev->ip_ptr));
WARN_ON(rcu_access_pointer(dev->ip6_ptr));
 
+   WARN_ON(timer_shutdown_sync(>watchdog_timer));
+
if (dev->priv_destructor)
dev->priv_destructor(dev);
if (dev->needs_free_netdev)

to catch how that watchdog_timer is getting queued. Would that make
sense, Eric?

[PATCH net-next] genetlink: start to validate reserved header bytes

2022-08-24 Thread Jakub Kicinski 
We had historically not checked that genlmsghdr.reserved
is 0 on input which prevents us from using those precious
bytes in the future.

One use case would be to extend the cmd field, which is
currently just 8 bits wide and 256 is not a lot of commands
for some core families.

To make sure that new families do the right thing by default
put the onus of opting out of validation on existing families.

Signed-off-by: Jakub Kicinski 
---
CC: j...@resnulli.us
CC: johan...@sipsolutions.net
CC: linux-bl...@vger.kernel.org
CC: osmocom-net-g...@lists.osmocom.org
CC: linux-w...@vger.kernel.org
CC: wireguard@lists.zx2c4.com
CC: linux-wirel...@vger.kernel.org
CC: linux-s...@vger.kernel.org
CC: target-de...@vger.kernel.org
CC: linux...@vger.kernel.org
CC: virtualizat...@lists.linux-foundation.org
CC: linux-c...@vger.kernel.org
CC: cluster-de...@redhat.com
CC: mp...@lists.linux.dev
CC: lvs-de...@vger.kernel.org
CC: netfilter-de...@vger.kernel.org
CC: linux-security-mod...@vger.kernel.org
CC: d...@openvswitch.org
CC: linux-s...@vger.kernel.org
CC: tipc-discuss...@lists.sourceforge.net
---
 drivers/block/nbd.c  | 1 +
 drivers/net/gtp.c| 1 +
 drivers/net/ieee802154/mac802154_hwsim.c | 1 +
 drivers/net/macsec.c | 1 +
 drivers/net/team/team.c  | 1 +
 drivers/net/wireguard/netlink.c  | 1 +
 drivers/net/wireless/mac80211_hwsim.c| 1 +
 drivers/target/target_core_user.c| 1 +
 drivers/thermal/thermal_netlink.c| 1 +
 drivers/vdpa/vdpa.c  | 1 +
 fs/cifs/netlink.c| 1 +
 fs/dlm/netlink.c | 1 +
 fs/ksmbd/transport_ipc.c | 1 +
 include/linux/genl_magic_func.h  | 1 +
 include/net/genetlink.h  | 3 +++
 kernel/taskstats.c   | 1 +
 net/batman-adv/netlink.c | 1 +
 net/core/devlink.c   | 1 +
 net/core/drop_monitor.c  | 1 +
 net/ethtool/netlink.c| 1 +
 net/hsr/hsr_netlink.c| 1 +
 net/ieee802154/netlink.c | 1 +
 net/ieee802154/nl802154.c| 1 +
 net/ipv4/fou.c   | 1 +
 net/ipv4/tcp_metrics.c   | 1 +
 net/ipv6/ila/ila_main.c  | 1 +
 net/ipv6/ioam6.c | 1 +
 net/ipv6/seg6.c  | 1 +
 net/l2tp/l2tp_netlink.c  | 1 +
 net/mptcp/pm_netlink.c   | 1 +
 net/ncsi/ncsi-netlink.c  | 1 +
 net/netfilter/ipvs/ip_vs_ctl.c   | 1 +
 net/netlabel/netlabel_calipso.c  | 1 +
 net/netlabel/netlabel_cipso_v4.c | 1 +
 net/netlabel/netlabel_mgmt.c | 1 +
 net/netlabel/netlabel_unlabeled.c| 1 +
 net/netlink/genetlink.c  | 4 
 net/nfc/netlink.c| 1 +
 net/openvswitch/conntrack.c  | 1 +
 net/openvswitch/datapath.c   | 3 +++
 net/openvswitch/meter.c  | 1 +
 net/psample/psample.c| 1 +
 net/smc/smc_netlink.c| 3 ++-
 net/smc/smc_pnet.c   | 3 ++-
 net/tipc/netlink.c   | 1 +
 net/tipc/netlink_compat.c| 1 +
 net/wireless/nl80211.c   | 1 +
 47 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 2a709daefbc4..6cec9ce23fd3 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -2322,6 +2322,7 @@ static struct genl_family nbd_genl_family __ro_after_init 
= {
.module = THIS_MODULE,
.small_ops  = nbd_connect_genl_ops,
.n_small_ops= ARRAY_SIZE(nbd_connect_genl_ops),
+   .resv_start_op  = NBD_CMD_STATUS + 1,
.maxattr= NBD_ATTR_MAX,
.policy = nbd_attr_policy,
.mcgrps = nbd_mcast_grps,
diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c
index a208e2b1a9af..15c7dc82107f 100644
--- a/drivers/net/gtp.c
+++ b/drivers/net/gtp.c
@@ -1859,6 +1859,7 @@ static struct genl_family gtp_genl_family __ro_after_init 
= {
.module = THIS_MODULE,
.small_ops  = gtp_genl_ops,
.n_small_ops= ARRAY_SIZE(gtp_genl_ops),
+   .resv_start_op  = GTP_CMD_ECHOREQ + 1,
.mcgrps = gtp_genl_mcgrps,
.n_mcgrps   = ARRAY_SIZE(gtp_genl_mcgrps),
 };
diff --git a/drivers/net/ieee802154/mac802154_hwsim.c 
b/drivers/net/ieee802154/mac802154_hwsim.c
index 38c217bd7c82..2f0544dd7c2a 100644
--- a/drivers/net/ieee802154/mac802154_hwsim.c
+++ b/drivers/net/ieee802154/mac802154_hwsim.c
@@ -630,6 +630,7 @@ static struct genl_family hwsim_genl_family __ro_after_init 
= {
.module = THIS_MODULE,
.small_ops = hwsim_nl_ops,
.n_small_ops = ARRAY_SIZE(hwsim_nl_ops),
+   .resv_start_op = MAC802154_HWSIM_CMD_NEW_EDGE + 1,
.mcgrps = hwsim_mcgrps,
.n_mcgrps

Re: [syzbot] linux-next test error: WARNING in __napi_schedule

2022-03-21 Thread Jakub Kicinski 
On Sat, 19 Mar 2022 01:16:24 -0700 syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:6d72dda014a4 Add linux-next specific files for 20220318
> git tree:   linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=124f558970
> kernel config:  https://syzkaller.appspot.com/x/.config?x=5907d82c35688f04
> dashboard link: https://syzkaller.appspot.com/bug?extid=6f21ac9e27fca7e97623
> compiler:   gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils 
> for Debian) 2.35.2

#syz fix: net: Revert the softirq will run annotation in napi_schedule().

Re: [syzbot] net-next test error: WARNING in __napi_schedule

2022-03-21 Thread Jakub Kicinski 
On Fri, 18 Mar 2022 16:36:19 -0700 syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:e89600ebeeb1 af_vsock: SOCK_SEQPACKET broken buffer test
> git tree:   net-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=134d43d570
> kernel config:  https://syzkaller.appspot.com/x/.config?x=ef691629edb94d6a
> dashboard link: https://syzkaller.appspot.com/bug?extid=fb57d2a7c4678481a495
> compiler:   gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils 
> for Debian) 2.35.2

#syz fix: net: Revert the softirq will run annotation in napi_schedule().

Re: [PATCH] net: remove lockdep asserts from ____napi_schedule()

2022-03-18 Thread Jakub Kicinski 
On Fri, 18 Mar 2022 18:50:08 -0600 Jason A. Donenfeld wrote:
> Hi Jakub,
> 
> Er, I forgot to mark this as net-next, but as it's connected to the
> discussion we were just having, I think you get the idea. :)

Yup, patchwork bot figured it out, too. All good :)

Re: [PATCH] skbuff: Switch structure bounds to struct_group()

2021-11-19 Thread Jakub Kicinski 
On Fri, 19 Nov 2021 10:41:44 -0800 Jakub Kicinski wrote:
> On Fri, 19 Nov 2021 10:26:19 -0800 Kees Cook wrote:
> > On Thu, Nov 18, 2021 at 11:13:55PM -0800, Jakub Kicinski wrote:  
> > > This adds ~27k of these warnings to W=1 gcc builds:
> > > 
> > > include/linux/skbuff.h:851:1: warning: directive in macro's argument list 
> > >
> > 
> > Hrm, I can't reproduce this, using several GCC versions and net-next. What
> > compiler version[1] and base commit[2] were used here?  
> 
> gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC) 
> 
> HEAD was at: 3b1abcf12894 Merge tag 'regmap-no-bus-update-bits' of git://...

Ah, damn, I just realized, it's probably sparse! The build sets C=1.

Re: [PATCH] skbuff: Switch structure bounds to struct_group()

2021-11-19 Thread Jakub Kicinski 
On Fri, 19 Nov 2021 10:26:19 -0800 Kees Cook wrote:
> On Thu, Nov 18, 2021 at 11:13:55PM -0800, Jakub Kicinski wrote:
> > On Thu, 18 Nov 2021 10:36:15 -0800 Kees Cook wrote:  
> > > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > > field bounds checking for memcpy(), memmove(), and memset(), avoid
> > > intentionally writing across neighboring fields.
> > > 
> > > Replace the existing empty member position markers "headers_start" and
> > > "headers_end" with a struct_group(). This will allow memcpy() and sizeof()
> > > to more easily reason about sizes, and improve readability.
> > > 
> > > "pahole" shows no size nor member offset changes to struct sk_buff.
> > > "objdump -d" shows no object code changes (outside of WARNs affected by
> > > source line number changes).  
> > 
> > This adds ~27k of these warnings to W=1 gcc builds:
> > 
> > include/linux/skbuff.h:851:1: warning: directive in macro's argument list  
> 
> Hrm, I can't reproduce this, using several GCC versions and net-next. What
> compiler version[1] and base commit[2] were used here?

gcc version 11.2.1 20210728 (Red Hat 11.2.1-1) (GCC) 

HEAD was at: 3b1abcf12894 Merge tag 'regmap-no-bus-update-bits' of git://...

> [1] https://github.com/kuba-moo/nipa/pull/10
> [2] https://github.com/kuba-moo/nipa/pull/11

Thanks for these! Will pull in as soon as the bot finishes with what
it's chewing on right now.

Re: [PATCH] skbuff: Switch structure bounds to struct_group()

2021-11-18 Thread Jakub Kicinski 
On Thu, 18 Nov 2021 10:36:15 -0800 Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memcpy(), memmove(), and memset(), avoid
> intentionally writing across neighboring fields.
> 
> Replace the existing empty member position markers "headers_start" and
> "headers_end" with a struct_group(). This will allow memcpy() and sizeof()
> to more easily reason about sizes, and improve readability.
> 
> "pahole" shows no size nor member offset changes to struct sk_buff.
> "objdump -d" shows no object code changes (outside of WARNs affected by
> source line number changes).

This adds ~27k of these warnings to W=1 gcc builds:

include/linux/skbuff.h:851:1: warning: directive in macro's argument list

Re: [PATCH net-next v3 00/10] net: add and use dev_get_tstats64

2020-11-09 Thread Jakub Kicinski 
On Sat, 7 Nov 2020 21:48:13 +0100 Heiner Kallweit wrote:
> It's a frequent pattern to use netdev->stats for the less frequently
> accessed counters and per-cpu counters for the frequently accessed
> counters (rx/tx bytes/packets). Add a default ndo_get_stats64()
> implementation for this use case. Subsequently switch more drivers
> to use this pattern.
> 
> v2:
> - add patches for replacing ip_tunnel_get_stats64
>   Requested additional migrations will come in a separate series.
> 
> v3:
> - add atomic_long_t member rx_frame_errors in patch 3 for making
>   counter updates atomic

Applied, thank you!

Re: [PATCH net-next v2 03/10] tun: switch to net core provided statistics counters

2020-11-06 Thread Jakub Kicinski 
On Fri, 6 Nov 2020 09:27:45 +0100 Heiner Kallweit wrote:
> On 06.11.2020 08:48, Heiner Kallweit wrote:
> > On 06.11.2020 02:14, Jakub Kicinski wrote:  
> >> On Wed, 4 Nov 2020 15:25:24 +0100 Heiner Kallweit wrote:  
> >>> @@ -1066,7 +1054,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff 
> >>> *skb, struct net_device *dev)
> >>>   return NETDEV_TX_OK;
> >>>  
> >>>  drop:
> >>> - this_cpu_inc(tun->pcpu_stats->tx_dropped);
> >>> + dev->stats.tx_dropped++;
> >>>   skb_tx_error(skb);
> >>>   kfree_skb(skb);
> >>>   rcu_read_unlock();  
> >>
> >> This is no longer atomic. Multiple CPUs may try to update it at the
> >> same time.
> >>
> >> Do you know what the story on dev->rx_dropped is? The kdoc says drivers
> >> are not supposed to use it but:
> >>
> >> drivers/net/ipvlan/ipvlan_core.c:   
> >> atomic_long_inc(>dev->rx_dropped);
> >> drivers/net/macvlan.c:  atomic_long_inc(>dev->rx_dropped);
> >> drivers/net/vxlan.c:atomic_long_inc(>dev->rx_dropped);
> >>
> >> Maybe tun can use it, too?
> >>  
> > Thanks, yes that should be possible. Here we speak about tx_dropped,
> > but AFAICS the same applies as for rx_dropped. Will change it accordingly
> > in a v3.
> >   
> For rx_dropped and tx_dropped it's easy, however tun also has a per-cpu
> counter for rx_frame_errors that is incremented if virtio_net_hdr_to_skb()
> fails. Not sure how to deal best with this one.

Umpf, yeah, so I'd probably add an atomic long to struct tun_struct,
but then you'll need to keep the ndo implementation instead of using
dev_get_tstats64 directly. I can't think of a better way, tho.

Re: [PATCH net-next v2 03/10] tun: switch to net core provided statistics counters

2020-11-05 Thread Jakub Kicinski 
On Wed, 4 Nov 2020 15:25:24 +0100 Heiner Kallweit wrote:
> @@ -1066,7 +1054,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, 
> struct net_device *dev)
>   return NETDEV_TX_OK;
>  
>  drop:
> - this_cpu_inc(tun->pcpu_stats->tx_dropped);
> + dev->stats.tx_dropped++;
>   skb_tx_error(skb);
>   kfree_skb(skb);
>   rcu_read_unlock();

This is no longer atomic. Multiple CPUs may try to update it at the
same time.

Do you know what the story on dev->rx_dropped is? The kdoc says drivers
are not supposed to use it but:

drivers/net/ipvlan/ipvlan_core.c:   
atomic_long_inc(>dev->rx_dropped);
drivers/net/macvlan.c:  atomic_long_inc(>dev->rx_dropped);
drivers/net/vxlan.c:atomic_long_inc(>dev->rx_dropped);

Maybe tun can use it, too?