[PATCH 0/3] using guard/__free in networking

[PATCH 0/3] using guard/__free in networking

[Johannes Berg]

Hi,

So I started playing with this for wifi, and overall that
does look pretty nice, but it's a bit weird if we can do

  guard(wiphy)(&rdev->wiphy);

or so, but still have to manually handle the RTNL in the
same code.

Hence these patches. The third one is a bit more RFC than
the others, I guess. It's also not tested, I'm not sure I
even know how to hit all the BPF parts.

johannes

[PATCH 1/3] rtnetlink: add guard for RTNL

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The new guard/scoped_gard can be useful for the RTNL as well,
so add a guard definition for it. It gets used like

 {
   guard(rtnl)();
   // RTNL held until end of block
 }

or

  scoped_guard(rtnl) {
    // RTNL held in this block
  }

as with any other guard/scoped_guard.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/linux/rtnetlink.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index cdfc897f1e3c..a7da7dfc06a2 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -7,6 +7,7 @@
 #include <linux/netdevice.h>
 #include <linux/wait.h>
 #include <linux/refcount.h>
+#include <linux/cleanup.h>
 #include <uapi/linux/rtnetlink.h>
 
 extern int rtnetlink_send(struct sk_buff *skb, struct net *net, u32 pid, u32 group, int echo);
@@ -46,6 +47,8 @@ extern int rtnl_is_locked(void);
 extern int rtnl_lock_killable(void);
 extern bool refcount_dec_and_rtnl_lock(refcount_t *r);
 
+DEFINE_LOCK_GUARD_0(rtnl, rtnl_lock(), rtnl_unlock())
+
 extern wait_queue_head_t netdev_unregistering_wq;
 extern atomic_t dev_unreg_count;
 extern struct rw_semaphore pernet_ops_rwsem;
-- 
2.44.0

[PATCH 2/3] netdevice: add DEFINE_FREE() for dev_put

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

For short netdev holds within a function there are still a lot of
users of dev_put() rather than netdev_put(). Add DEFINE_FREE() to
allow making those safer.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/linux/netdevice.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index cb37817d6382..f6c0d731fa35 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4127,6 +4127,8 @@ static inline void dev_put(struct net_device *dev)
 	netdev_put(dev, NULL);
 }
 
+DEFINE_FREE(dev_put, struct net_device *, if (_T) dev_put(_T))
+
 static inline void netdev_ref_replace(struct net_device *odev,
 				      struct net_device *ndev,
 				      netdevice_tracker *tracker,
-- 
2.44.0

[PATCH 3/3] net: core: use guard/__free in core dev code

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Simplify the code a bit, mostly also to illustrate
the new helpers.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/core/dev.c | 182 +++++++++++++++++++------------------------------
 1 file changed, 72 insertions(+), 110 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 9a67003e49db..f4f4627bb1e3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1367,9 +1367,8 @@ EXPORT_SYMBOL(__netdev_notify_peers);
  */
 void netdev_notify_peers(struct net_device *dev)
 {
-	rtnl_lock();
+	guard(rtnl)();
 	__netdev_notify_peers(dev);
-	rtnl_unlock();
 }
 EXPORT_SYMBOL(netdev_notify_peers);
 
@@ -1722,30 +1721,27 @@ int register_netdevice_notifier(struct notifier_block *nb)
 	int err;
 
 	/* Close race with setup_net() and cleanup_net() */
-	down_write(&pernet_ops_rwsem);
-	rtnl_lock();
+	guard(rwsem_write)(&pernet_ops_rwsem);
+	guard(rtnl)();
 	err = raw_notifier_chain_register(&netdev_chain, nb);
 	if (err)
-		goto unlock;
+		return err;
 	if (dev_boot_phase)
-		goto unlock;
+		return 0;
 	for_each_net(net) {
 		err = call_netdevice_register_net_notifiers(nb, net);
 		if (err)
 			goto rollback;
 	}
 
-unlock:
-	rtnl_unlock();
-	up_write(&pernet_ops_rwsem);
-	return err;
+	return 0;
 
 rollback:
 	for_each_net_continue_reverse(net)
 		call_netdevice_unregister_net_notifiers(nb, net);
 
 	raw_notifier_chain_unregister(&netdev_chain, nb);
-	goto unlock;
+	return err;
 }
 EXPORT_SYMBOL(register_netdevice_notifier);
 
@@ -1769,19 +1765,16 @@ int unregister_netdevice_notifier(struct notifier_block *nb)
 	int err;
 
 	/* Close race with setup_net() and cleanup_net() */
-	down_write(&pernet_ops_rwsem);
-	rtnl_lock();
+	guard(rwsem_write)(&pernet_ops_rwsem);
+	guard(rtnl)();
 	err = raw_notifier_chain_unregister(&netdev_chain, nb);
 	if (err)
-		goto unlock;
+		return err;
 
 	for_each_net(net)
 		call_netdevice_unregister_net_notifiers(nb, net);
 
-unlock:
-	rtnl_unlock();
-	up_write(&pernet_ops_rwsem);
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(unregister_netdevice_notifier);
 
@@ -1838,12 +1831,9 @@ static int __unregister_netdevice_notifier_net(struct net *net,
 
 int register_netdevice_notifier_net(struct net *net, struct notifier_block *nb)
 {
-	int err;
+	guard(rtnl)();
 
-	rtnl_lock();
-	err = __register_netdevice_notifier_net(net, nb, false);
-	rtnl_unlock();
-	return err;
+	return __register_netdevice_notifier_net(net, nb, false);
 }
 EXPORT_SYMBOL(register_netdevice_notifier_net);
 
@@ -1866,12 +1856,9 @@ EXPORT_SYMBOL(register_netdevice_notifier_net);
 int unregister_netdevice_notifier_net(struct net *net,
 				      struct notifier_block *nb)
 {
-	int err;
+	guard(rtnl)();
 
-	rtnl_lock();
-	err = __unregister_netdevice_notifier_net(net, nb);
-	rtnl_unlock();
-	return err;
+	return __unregister_netdevice_notifier_net(net, nb);
 }
 EXPORT_SYMBOL(unregister_netdevice_notifier_net);
 
@@ -1889,14 +1876,14 @@ int register_netdevice_notifier_dev_net(struct net_device *dev,
 {
 	int err;
 
-	rtnl_lock();
+	guard(rtnl)();
 	err = __register_netdevice_notifier_net(dev_net(dev), nb, false);
-	if (!err) {
-		nn->nb = nb;
-		list_add(&nn->list, &dev->net_notifier_list);
-	}
-	rtnl_unlock();
-	return err;
+	if (err)
+		return err;
+
+	nn->nb = nb;
+	list_add(&nn->list, &dev->net_notifier_list);
+	return 0;
 }
 EXPORT_SYMBOL(register_netdevice_notifier_dev_net);
 
@@ -1904,13 +1891,10 @@ int unregister_netdevice_notifier_dev_net(struct net_device *dev,
 					  struct notifier_block *nb,
 					  struct netdev_net_notifier *nn)
 {
-	int err;
+	guard(rtnl)();
 
-	rtnl_lock();
 	list_del(&nn->list);
-	err = __unregister_netdevice_notifier_net(dev_net(dev), nb);
-	rtnl_unlock();
-	return err;
+	return __unregister_netdevice_notifier_net(dev_net(dev), nb);
 }
 EXPORT_SYMBOL(unregister_netdevice_notifier_dev_net);
 
@@ -9453,7 +9437,7 @@ static void bpf_xdp_link_release(struct bpf_link *link)
 {
 	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
 
-	rtnl_lock();
+	guard(rtnl)();
 
 	/* if racing with net_device's tear down, xdp_link->dev might be
 	 * already NULL, in which case link was already auto-detached
@@ -9462,8 +9446,6 @@ static void bpf_xdp_link_release(struct bpf_link *link)
 		WARN_ON(dev_xdp_detach_link(xdp_link->dev, NULL, xdp_link));
 		xdp_link->dev = NULL;
 	}
-
-	rtnl_unlock();
 }
 
 static int bpf_xdp_link_detach(struct bpf_link *link)
@@ -9485,10 +9467,10 @@ static void bpf_xdp_link_show_fdinfo(const struct bpf_link *link,
 	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
 	u32 ifindex = 0;
 
-	rtnl_lock();
-	if (xdp_link->dev)
-		ifindex = xdp_link->dev->ifindex;
-	rtnl_unlock();
+	scoped_guard(rtnl) {
+		if (xdp_link->dev)
+			ifindex = xdp_link->dev->ifindex;
+	}
 
 	seq_printf(seq, "ifindex:\t%u\n", ifindex);
 }
@@ -9499,10 +9481,10 @@ static int bpf_xdp_link_fill_link_info(const struct bpf_link *link,
 	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
 	u32 ifindex = 0;
 
-	rtnl_lock();
-	if (xdp_link->dev)
-		ifindex = xdp_link->dev->ifindex;
-	rtnl_unlock();
+	scoped_guard(rtnl) {
+		if (xdp_link->dev)
+			ifindex = xdp_link->dev->ifindex;
+	}
 
 	info->xdp.ifindex = ifindex;
 	return 0;
@@ -9514,31 +9496,26 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
 	struct bpf_xdp_link *xdp_link = container_of(link, struct bpf_xdp_link, link);
 	enum bpf_xdp_mode mode;
 	bpf_op_t bpf_op;
-	int err = 0;
+	int err;
 
-	rtnl_lock();
+	guard(rtnl)();
 
 	/* link might have been auto-released already, so fail */
-	if (!xdp_link->dev) {
-		err = -ENOLINK;
-		goto out_unlock;
-	}
+	if (!xdp_link->dev)
+		return -ENOLINK;
+
+	if (old_prog && link->prog != old_prog)
+		return -EPERM;
 
-	if (old_prog && link->prog != old_prog) {
-		err = -EPERM;
-		goto out_unlock;
-	}
 	old_prog = link->prog;
 	if (old_prog->type != new_prog->type ||
-	    old_prog->expected_attach_type != new_prog->expected_attach_type) {
-		err = -EINVAL;
-		goto out_unlock;
-	}
+	    old_prog->expected_attach_type != new_prog->expected_attach_type)
+		return -EINVAL;
 
 	if (old_prog == new_prog) {
 		/* no-op, don't disturb drivers */
 		bpf_prog_put(new_prog);
-		goto out_unlock;
+		return 0;
 	}
 
 	mode = dev_xdp_mode(xdp_link->dev, xdp_link->flags);
@@ -9546,14 +9523,11 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
 	err = dev_xdp_install(xdp_link->dev, mode, bpf_op, NULL,
 			      xdp_link->flags, new_prog);
 	if (err)
-		goto out_unlock;
+		return err;
 
 	old_prog = xchg(&link->prog, new_prog);
 	bpf_prog_put(old_prog);
-
-out_unlock:
-	rtnl_unlock();
-	return err;
+	return 0;
 }
 
 static const struct bpf_link_ops bpf_xdp_link_lops = {
@@ -9567,57 +9541,47 @@ static const struct bpf_link_ops bpf_xdp_link_lops = {
 
 int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
+	/* link itself doesn't hold dev's refcnt to not complicate shutdown */
+	struct net_device *dev __free(dev_put) = NULL;
 	struct net *net = current->nsproxy->net_ns;
 	struct bpf_link_primer link_primer;
 	struct netlink_ext_ack extack = {};
 	struct bpf_xdp_link *link;
-	struct net_device *dev;
-	int err, fd;
+	int err;
 
-	rtnl_lock();
-	dev = dev_get_by_index(net, attr->link_create.target_ifindex);
-	if (!dev) {
-		rtnl_unlock();
-		return -EINVAL;
-	}
+	scoped_guard(rtnl) {
+		dev = dev_get_by_index(net,
+				       attr->link_create.target_ifindex);
+		if (!dev)
+			return -EINVAL;
 
-	link = kzalloc(sizeof(*link), GFP_USER);
-	if (!link) {
-		err = -ENOMEM;
-		goto unlock;
-	}
+		link = kzalloc(sizeof(*link), GFP_USER);
+		if (!link)
+			return -ENOMEM;
 
-	bpf_link_init(&link->link, BPF_LINK_TYPE_XDP, &bpf_xdp_link_lops, prog);
-	link->dev = dev;
-	link->flags = attr->link_create.flags;
+		bpf_link_init(&link->link, BPF_LINK_TYPE_XDP,
+			      &bpf_xdp_link_lops, prog);
 
-	err = bpf_link_prime(&link->link, &link_primer);
-	if (err) {
-		kfree(link);
-		goto unlock;
-	}
+		link->dev = dev;
+		link->flags = attr->link_create.flags;
 
-	err = dev_xdp_attach_link(dev, &extack, link);
-	rtnl_unlock();
+		err = bpf_link_prime(&link->link, &link_primer);
+		if (err) {
+			kfree(link);
+			return err;
+		}
+
+		err = dev_xdp_attach_link(dev, &extack, link);
+	}
 
 	if (err) {
 		link->dev = NULL;
 		bpf_link_cleanup(&link_primer);
 		trace_bpf_xdp_link_attach_failed(extack._msg);
-		goto out_put_dev;
+		return err;
 	}
 
-	fd = bpf_link_settle(&link_primer);
-	/* link itself doesn't hold dev's refcnt to not complicate shutdown */
-	dev_put(dev);
-	return fd;
-
-unlock:
-	rtnl_unlock();
-
-out_put_dev:
-	dev_put(dev);
-	return err;
+	return bpf_link_settle(&link_primer);
 }
 
 /**
@@ -11171,9 +11135,8 @@ EXPORT_SYMBOL(unregister_netdevice_many);
  */
 void unregister_netdev(struct net_device *dev)
 {
-	rtnl_lock();
+	guard(rtnl)();
 	unregister_netdevice(dev);
-	rtnl_unlock();
 }
 EXPORT_SYMBOL(unregister_netdev);
 
@@ -11617,7 +11580,7 @@ static void __net_exit default_device_exit_batch(struct list_head *net_list)
 	struct net *net;
 	LIST_HEAD(dev_kill_list);
 
-	rtnl_lock();
+	guard(rtnl)();
 	list_for_each_entry(net, net_list, exit_list) {
 		default_device_exit_net(net);
 		cond_resched();
@@ -11632,7 +11595,6 @@ static void __net_exit default_device_exit_batch(struct list_head *net_list)
 		}
 	}
 	unregister_netdevice_many(&dev_kill_list);
-	rtnl_unlock();
 }
 
 static struct pernet_operations __net_initdata default_device_ops = {
-- 
2.44.0

Re: [PATCH 0/3] using guard/__free in networking

[Jakub Kicinski]

On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:
> Hi,
> 
> So I started playing with this for wifi, and overall that
> does look pretty nice, but it's a bit weird if we can do
> 
>   guard(wiphy)(&rdev->wiphy);
> 
> or so, but still have to manually handle the RTNL in the
> same code.

Dunno, it locks code instead of data accesses.
Forgive the comparison but it feels too much like Java to me :)
scoped_guard is fine, the guard() not so much.

But happy for other netdev maintainers to override me..

Do you have a piece of code in wireless where the conversion
made you go "wow, this is so much cleaner"?

Re: [PATCH 0/3] using guard/__free in networking

[Johannes Berg]

On Mon, 2024-03-25 at 19:09 -0700, Jakub Kicinski wrote:
> On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:
> > Hi,
> > 
> > So I started playing with this for wifi, and overall that
> > does look pretty nice, but it's a bit weird if we can do
> > 
> >   guard(wiphy)(&rdev->wiphy);
> > 
> > or so, but still have to manually handle the RTNL in the
> > same code.
> 
> Dunno, it locks code instead of data accesses.

Well, I'm not sure that's a fair complaint. After all, without any more
compiler help, even rtnl_lock()/rtnl_unlock() _necessarily_ locks code.
Clearly

	rtnl_lock();
	// something
	rtnl_unlock();

also locks the "// something" code, after all., and yeah that might be
doing data accesses, but it might also be a function call or a whole
bunch of other things?

Or if you look at something like bpf_xdp_link_attach(), I don't think
you can really say that it locks only data. That doesn't even do the
allocation outside the lock (though I did convert that one to
scoped_guard because of that.)

Or even something simple like unregister_netdev(), it just requires the
RTNL for some data accesses and consistency deep inside
unregister_netdevice(), not for any specific data accessed there.

So yeah, this is always going to be a trade-off, but all the locking is.
We even make similar trade-offs manually, e.g. look at
bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
still, for no good reason other than simplifying the cleanup path there.


Anyway, I can live with it either way (unless you tell me you won't pull
wireless code using guard), just thought doing the wireless locking with
guard and the RTNL around it without it (only in a few places do we
still use RTNL though) looked odd.


> Forgive the comparison but it feels too much like Java to me :)

Heh. Haven't used Java in 20 years or so...

> scoped_guard is fine, the guard() not so much.

I think you can't get scoped_guard() without guard(), so does that mean
you'd accept the first patch in the series?

> Do you have a piece of code in wireless where the conversion
> made you go "wow, this is so much cleaner"?

Mostly long and complex error paths. Found a double-unlock bug (in
iwlwifi) too, when converting some locking there.

Doing a more broader conversion on cfg80211/mac80211 removes around 200
lines of unlocking, mostly error handling, code.

Doing __free() too will probably clean up even more.

johannes

Re: [PATCH 0/3] using guard/__free in networking

[Jakub Kicinski]

On Tue, 26 Mar 2024 09:42:43 +0100 Johannes Berg wrote:
> On Mon, 2024-03-25 at 19:09 -0700, Jakub Kicinski wrote:
> > On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:  
> > > Hi,
> > > 
> > > So I started playing with this for wifi, and overall that
> > > does look pretty nice, but it's a bit weird if we can do
> > > 
> > >   guard(wiphy)(&rdev->wiphy);
> > > 
> > > or so, but still have to manually handle the RTNL in the
> > > same code.  
> > 
> > Dunno, it locks code instead of data accesses.  
> 
> Well, I'm not sure that's a fair complaint. After all, without any more
> compiler help, even rtnl_lock()/rtnl_unlock() _necessarily_ locks code.
> Clearly
> 
> 	rtnl_lock();
> 	// something
> 	rtnl_unlock();
> 
> also locks the "// something" code, after all., and yeah that might be
> doing data accesses, but it might also be a function call or a whole
> bunch of other things?
> 
> Or if you look at something like bpf_xdp_link_attach(), I don't think
> you can really say that it locks only data. That doesn't even do the
> allocation outside the lock (though I did convert that one to
> scoped_guard because of that.)
> 
> Or even something simple like unregister_netdev(), it just requires the
> RTNL for some data accesses and consistency deep inside
> unregister_netdevice(), not for any specific data accessed there.
> 
> So yeah, this is always going to be a trade-off, but all the locking is.
> We even make similar trade-offs manually, e.g. look at
> bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
> still, for no good reason other than simplifying the cleanup path there.

At least to me the mental model is different. 99% of the time the guard
is covering the entire body. So now we're moving from "I'm touching X
so I need to lock" to "This _function_ is safe to touch X".

> Anyway, I can live with it either way (unless you tell me you won't pull
> wireless code using guard), just thought doing the wireless locking with
> guard and the RTNL around it without it (only in a few places do we
> still use RTNL though) looked odd.
> 
> 
> > Forgive the comparison but it feels too much like Java to me :)  
> 
> Heh. Haven't used Java in 20 years or so...

I only did at uni, but I think they had a decorator for a method, where
you can basically say "this method should be under lock X" and runtime
will take that lock before entering and drop it after exit,
appropriately. I wonder why the sudden love for this concept :S
Is it also present in Rust or some such?

> > scoped_guard is fine, the guard() not so much.  
> 
> I think you can't get scoped_guard() without guard(), so does that mean
> you'd accept the first patch in the series?

How can we get one without the other.. do you reckon Joe P would let us
add a checkpatch check to warn people against pure guard() under net/ ?

> > Do you have a piece of code in wireless where the conversion
> > made you go "wow, this is so much cleaner"?  
> 
> Mostly long and complex error paths. Found a double-unlock bug (in
> iwlwifi) too, when converting some locking there.
> 
> Doing a more broader conversion on cfg80211/mac80211 removes around 200
> lines of unlocking, mostly error handling, code.
> 
> Doing __free() too will probably clean up even more.

Not super convinced by that one either:
https://lore.kernel.org/all/20240321185640.6f7f4d6b@kernel.org/
maybe I'm too conservative..

Re: [PATCH 0/3] using guard/__free in networking

[Willem de Bruijn]

Jakub Kicinski wrote:
> On Tue, 26 Mar 2024 09:42:43 +0100 Johannes Berg wrote:
> > On Mon, 2024-03-25 at 19:09 -0700, Jakub Kicinski wrote:
> > > On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:  
> > > > Hi,
> > > > 
> > > > So I started playing with this for wifi, and overall that
> > > > does look pretty nice, but it's a bit weird if we can do
> > > > 
> > > >   guard(wiphy)(&rdev->wiphy);
> > > > 
> > > > or so, but still have to manually handle the RTNL in the
> > > > same code.  
> > > 
> > > Dunno, it locks code instead of data accesses.  
> > 
> > Well, I'm not sure that's a fair complaint. After all, without any more
> > compiler help, even rtnl_lock()/rtnl_unlock() _necessarily_ locks code.
> > Clearly
> > 
> > 	rtnl_lock();
> > 	// something
> > 	rtnl_unlock();
> > 
> > also locks the "// something" code, after all., and yeah that might be
> > doing data accesses, but it might also be a function call or a whole
> > bunch of other things?
> > 
> > Or if you look at something like bpf_xdp_link_attach(), I don't think
> > you can really say that it locks only data. That doesn't even do the
> > allocation outside the lock (though I did convert that one to
> > scoped_guard because of that.)
> > 
> > Or even something simple like unregister_netdev(), it just requires the
> > RTNL for some data accesses and consistency deep inside
> > unregister_netdevice(), not for any specific data accessed there.
> > 
> > So yeah, this is always going to be a trade-off, but all the locking is.
> > We even make similar trade-offs manually, e.g. look at
> > bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
> > still, for no good reason other than simplifying the cleanup path there.
> 
> At least to me the mental model is different. 99% of the time the guard
> is covering the entire body. So now we're moving from "I'm touching X
> so I need to lock" to "This _function_ is safe to touch X".
> 
> > Anyway, I can live with it either way (unless you tell me you won't pull
> > wireless code using guard), just thought doing the wireless locking with
> > guard and the RTNL around it without it (only in a few places do we
> > still use RTNL though) looked odd.
> > 
> > 
> > > Forgive the comparison but it feels too much like Java to me :)  
> > 
> > Heh. Haven't used Java in 20 years or so...
> 
> I only did at uni, but I think they had a decorator for a method, where
> you can basically say "this method should be under lock X" and runtime
> will take that lock before entering and drop it after exit,
> appropriately. I wonder why the sudden love for this concept :S
> Is it also present in Rust or some such?
> 
> > > scoped_guard is fine, the guard() not so much.  
> > 
> > I think you can't get scoped_guard() without guard(), so does that mean
> > you'd accept the first patch in the series?
> 
> How can we get one without the other.. do you reckon Joe P would let us
> add a checkpatch check to warn people against pure guard() under net/ ?
> 
> > > Do you have a piece of code in wireless where the conversion
> > > made you go "wow, this is so much cleaner"?  
> > 
> > Mostly long and complex error paths. Found a double-unlock bug (in
> > iwlwifi) too, when converting some locking there.
> > 
> > Doing a more broader conversion on cfg80211/mac80211 removes around 200
> > lines of unlocking, mostly error handling, code.
> > 
> > Doing __free() too will probably clean up even more.
> 
> Not super convinced by that one either:
> https://lore.kernel.org/all/20240321185640.6f7f4d6b@kernel.org/
> maybe I'm too conservative..

+1 on the concept (fwiw).

Even the simple examples, such as unregister_netdevice_notifier_net,
show how it avoids boilerplate and so simplifies control flow.

That benefit multiplies with the number of resources held and number
of exit paths. Or in our case, gotos and (unlock) labels.

Error paths are notorious for seeing little test coverage and leaking
resources. This is an easy class of bugs that this RAII squashes.

Sprinkling guard statements anywhere in the scope itself makes it
perhaps hard to follow. Perhaps a heuristic would be to require these
statements at the start of scope (after variable declaration)?

Function level decorators could further inform static analysis.
But that is somewhat tangential.

Re: [PATCH 0/3] using guard/__free in networking

[Stanislav Fomichev]

On 03/26, Willem de Bruijn wrote:
> Jakub Kicinski wrote:
> > On Tue, 26 Mar 2024 09:42:43 +0100 Johannes Berg wrote:
> > > On Mon, 2024-03-25 at 19:09 -0700, Jakub Kicinski wrote:
> > > > On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:  
> > > > > Hi,
> > > > > 
> > > > > So I started playing with this for wifi, and overall that
> > > > > does look pretty nice, but it's a bit weird if we can do
> > > > > 
> > > > >   guard(wiphy)(&rdev->wiphy);
> > > > > 
> > > > > or so, but still have to manually handle the RTNL in the
> > > > > same code.  
> > > > 
> > > > Dunno, it locks code instead of data accesses.  
> > > 
> > > Well, I'm not sure that's a fair complaint. After all, without any more
> > > compiler help, even rtnl_lock()/rtnl_unlock() _necessarily_ locks code.
> > > Clearly
> > > 
> > > 	rtnl_lock();
> > > 	// something
> > > 	rtnl_unlock();
> > > 
> > > also locks the "// something" code, after all., and yeah that might be
> > > doing data accesses, but it might also be a function call or a whole
> > > bunch of other things?
> > > 
> > > Or if you look at something like bpf_xdp_link_attach(), I don't think
> > > you can really say that it locks only data. That doesn't even do the
> > > allocation outside the lock (though I did convert that one to
> > > scoped_guard because of that.)
> > > 
> > > Or even something simple like unregister_netdev(), it just requires the
> > > RTNL for some data accesses and consistency deep inside
> > > unregister_netdevice(), not for any specific data accessed there.
> > > 
> > > So yeah, this is always going to be a trade-off, but all the locking is.
> > > We even make similar trade-offs manually, e.g. look at
> > > bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
> > > still, for no good reason other than simplifying the cleanup path there.
> > 
> > At least to me the mental model is different. 99% of the time the guard
> > is covering the entire body. So now we're moving from "I'm touching X
> > so I need to lock" to "This _function_ is safe to touch X".
> > 
> > > Anyway, I can live with it either way (unless you tell me you won't pull
> > > wireless code using guard), just thought doing the wireless locking with
> > > guard and the RTNL around it without it (only in a few places do we
> > > still use RTNL though) looked odd.
> > > 
> > > 
> > > > Forgive the comparison but it feels too much like Java to me :)  
> > > 
> > > Heh. Haven't used Java in 20 years or so...
> > 
> > I only did at uni, but I think they had a decorator for a method, where
> > you can basically say "this method should be under lock X" and runtime
> > will take that lock before entering and drop it after exit,
> > appropriately. I wonder why the sudden love for this concept :S
> > Is it also present in Rust or some such?

It's more of a c++ thing I believe: https://en.cppreference.com/w/cpp/thread/lock_guard

Not that anybody is asking my opinion (and my mind has been a bit corrupted
by c++), but guard() syntax seems fine :-p

Rust's approach is more conventional. There is a mtx.lock() method that
returns a scoped guard that can be optionally unlock'ed IIRC.

> > > > scoped_guard is fine, the guard() not so much.  
> > > 
> > > I think you can't get scoped_guard() without guard(), so does that mean
> > > you'd accept the first patch in the series?
> > 
> > How can we get one without the other.. do you reckon Joe P would let us
> > add a checkpatch check to warn people against pure guard() under net/ ?
> > 
> > > > Do you have a piece of code in wireless where the conversion
> > > > made you go "wow, this is so much cleaner"?  
> > > 
> > > Mostly long and complex error paths. Found a double-unlock bug (in
> > > iwlwifi) too, when converting some locking there.
> > > 
> > > Doing a more broader conversion on cfg80211/mac80211 removes around 200
> > > lines of unlocking, mostly error handling, code.
> > > 
> > > Doing __free() too will probably clean up even more.
> > 
> > Not super convinced by that one either:
> > https://lore.kernel.org/all/20240321185640.6f7f4d6b@kernel.org/
> > maybe I'm too conservative..
> 
> +1 on the concept (fwiw).
> 
> Even the simple examples, such as unregister_netdevice_notifier_net,
> show how it avoids boilerplate and so simplifies control flow.
> 
> That benefit multiplies with the number of resources held and number
> of exit paths. Or in our case, gotos and (unlock) labels.
> 
> Error paths are notorious for seeing little test coverage and leaking
> resources. This is an easy class of bugs that this RAII squashes.
> 
> Sprinkling guard statements anywhere in the scope itself makes it
> perhaps hard to follow. Perhaps a heuristic would be to require these
> statements at the start of scope (after variable declaration)?
> 
> Function level decorators could further inform static analysis.
> But that is somewhat tangential.

Re: [PATCH 0/3] using guard/__free in networking

[Simon Horman]

On Tue, Mar 26, 2024 at 11:23:19AM -0400, Willem de Bruijn wrote:
> Jakub Kicinski wrote:
> > On Tue, 26 Mar 2024 09:42:43 +0100 Johannes Berg wrote:
> > > On Mon, 2024-03-25 at 19:09 -0700, Jakub Kicinski wrote:
> > > > On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:  
> > > > > Hi,
> > > > > 
> > > > > So I started playing with this for wifi, and overall that
> > > > > does look pretty nice, but it's a bit weird if we can do
> > > > > 
> > > > >   guard(wiphy)(&rdev->wiphy);
> > > > > 
> > > > > or so, but still have to manually handle the RTNL in the
> > > > > same code.  
> > > > 
> > > > Dunno, it locks code instead of data accesses.  
> > > 
> > > Well, I'm not sure that's a fair complaint. After all, without any more
> > > compiler help, even rtnl_lock()/rtnl_unlock() _necessarily_ locks code.
> > > Clearly
> > > 
> > > 	rtnl_lock();
> > > 	// something
> > > 	rtnl_unlock();
> > > 
> > > also locks the "// something" code, after all., and yeah that might be
> > > doing data accesses, but it might also be a function call or a whole
> > > bunch of other things?
> > > 
> > > Or if you look at something like bpf_xdp_link_attach(), I don't think
> > > you can really say that it locks only data. That doesn't even do the
> > > allocation outside the lock (though I did convert that one to
> > > scoped_guard because of that.)
> > > 
> > > Or even something simple like unregister_netdev(), it just requires the
> > > RTNL for some data accesses and consistency deep inside
> > > unregister_netdevice(), not for any specific data accessed there.
> > > 
> > > So yeah, this is always going to be a trade-off, but all the locking is.
> > > We even make similar trade-offs manually, e.g. look at
> > > bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
> > > still, for no good reason other than simplifying the cleanup path there.
> > 
> > At least to me the mental model is different. 99% of the time the guard
> > is covering the entire body. So now we're moving from "I'm touching X
> > so I need to lock" to "This _function_ is safe to touch X".
> > 
> > > Anyway, I can live with it either way (unless you tell me you won't pull
> > > wireless code using guard), just thought doing the wireless locking with
> > > guard and the RTNL around it without it (only in a few places do we
> > > still use RTNL though) looked odd.
> > > 
> > > 
> > > > Forgive the comparison but it feels too much like Java to me :)  
> > > 
> > > Heh. Haven't used Java in 20 years or so...
> > 
> > I only did at uni, but I think they had a decorator for a method, where
> > you can basically say "this method should be under lock X" and runtime
> > will take that lock before entering and drop it after exit,
> > appropriately. I wonder why the sudden love for this concept :S
> > Is it also present in Rust or some such?
> > 
> > > > scoped_guard is fine, the guard() not so much.  
> > > 
> > > I think you can't get scoped_guard() without guard(), so does that mean
> > > you'd accept the first patch in the series?
> > 
> > How can we get one without the other.. do you reckon Joe P would let us
> > add a checkpatch check to warn people against pure guard() under net/ ?
> > 
> > > > Do you have a piece of code in wireless where the conversion
> > > > made you go "wow, this is so much cleaner"?  
> > > 
> > > Mostly long and complex error paths. Found a double-unlock bug (in
> > > iwlwifi) too, when converting some locking there.
> > > 
> > > Doing a more broader conversion on cfg80211/mac80211 removes around 200
> > > lines of unlocking, mostly error handling, code.
> > > 
> > > Doing __free() too will probably clean up even more.
> > 
> > Not super convinced by that one either:
> > https://lore.kernel.org/all/20240321185640.6f7f4d6b@kernel.org/
> > maybe I'm too conservative..
> 
> +1 on the concept (fwiw).
> 
> Even the simple examples, such as unregister_netdevice_notifier_net,
> show how it avoids boilerplate and so simplifies control flow.
> 
> That benefit multiplies with the number of resources held and number
> of exit paths. Or in our case, gotos and (unlock) labels.
> 
> Error paths are notorious for seeing little test coverage and leaking
> resources. This is an easy class of bugs that this RAII squashes.

+1

While I'm ambivalent to the constructs that have been proposed,
I do see leaking resources in error paths as a very common pattern.
Especially in new code. Making that easier to get right seems
worthwhile to me.

> 
> Sprinkling guard statements anywhere in the scope itself makes it
> perhaps hard to follow. Perhaps a heuristic would be to require these
> statements at the start of scope (after variable declaration)?
> 
> Function level decorators could further inform static analysis.
> But that is somewhat tangential.
> 

Re: [PATCH 0/3] using guard/__free in networking

[Johannes Berg]

> > So yeah, this is always going to be a trade-off, but all the locking is.
> > We even make similar trade-offs manually, e.g. look at
> > bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
> > still, for no good reason other than simplifying the cleanup path there.
> 
> At least to me the mental model is different. 99% of the time the guard
> is covering the entire body. So now we're moving from "I'm touching X
> so I need to lock" to "This _function_ is safe to touch X".

Yeah, maybe. But then we also have a lot of trivial wrappers just do to
locking, like

do_something()
{
	rtnl_lock()
	ret = __do_something()
	rtnl_unlock();
	return ret;
}

because __do_something() has a bunch of error paths and it's just messy
to maintain the locking directly :)

So I guess I don't have that much of a different model, I'd actually say
it's an advantage of sorts in many cases, and in some cases you'd still
have "rtnl_lock()" only in the middle, or scoped_guard(rtnl) {...} for a
small block. But refactoring a function because it's accessing protected
data doesn't seem completely out of the question either.

> > > Forgive the comparison but it feels too much like Java to me :)  
> > 
> > Heh. Haven't used Java in 20 years or so...
> 
> I only did at uni, but I think they had a decorator for a method, where
> you can basically say "this method should be under lock X" and runtime
> will take that lock before entering and drop it after exit,
> appropriately.

Yeah, I vaguely remember that. And yes, you can obviously just slap that
on everything and call it a day, or you could also there refactor the
things that do need the locking into a separate function, and use it
there?

> I wonder why the sudden love for this concept :S

I think it's neither sudden nor love ;-) But all the cleanup paths are
_always_ a mess to maintain, and this is the least bad thing folks came
up with so far?

> Is it also present in Rust or some such?

I have no idea. I _think_ Rust actually ties the data and the locks
together more?

> > > scoped_guard is fine, the guard() not so much.  
> > 
> > I think you can't get scoped_guard() without guard(), so does that mean
> > you'd accept the first patch in the series?
> 
> How can we get one without the other.. do you reckon Joe P would let us
> add a checkpatch check to warn people against pure guard() under net/ ?

Maybe? But I think I do want to use guard() ;-)

There are a ton of functions like say cfg80211_wext_siwrts() or
nl80211_new_interface() that really just want to hold the mutex for the
(remainder of) the function. And really _nl80211_new_interface()
wouldn't even exist if we had that, because nl80211_new_interface()
would just be

nl80211_new_interface()
{
  cfg80211_destroy_ifaces(rdev);

  guard(wiphy)(&rdev->wiphy);

  // exactly existing content of _nl80211_new_interface()
  // with all the returns etc.
}

the only reason for _nl80211_new_interface() is the locking there ...

Actually, we might push that further down into the function, to just
before rdev_add_virtual_intf(), I guess? But it all just adds to the
complexity as long as it's not cleaned up automatically.

> > > Do you have a piece of code in wireless where the conversion
> > > made you go "wow, this is so much cleaner"?  
> > 
> > Mostly long and complex error paths. Found a double-unlock bug (in
> > iwlwifi) too, when converting some locking there.
> > 
> > Doing a more broader conversion on cfg80211/mac80211 removes around 200
> > lines of unlocking, mostly error handling, code.
> > 
> > Doing __free() too will probably clean up even more.
> 
> Not super convinced by that one either:
> https://lore.kernel.org/all/20240321185640.6f7f4d6b@kernel.org/
> maybe I'm too conservative..

I mean ... it's not great, and it's all new stuff to learn (especially
those caveats with the cleanup order), but error paths are the things
that are really never tested and tend to be broken, no matter that we
have smatch/sparse/coverity/etc.

So while I don't think it's perfect, and I tend to agree even that it
encourages "over-locking" (though we can refactor and use scope etc.
where it matters), I'm pretty firmly on the "we should be using this to
not worry about error paths all the time" side of the fence, I guess.

johannes

Re: [PATCH 0/3] using guard/__free in networking

[Jakub Kicinski]

On Tue, 26 Mar 2024 16:33:58 +0100 Johannes Berg wrote:
> > > I think you can't get scoped_guard() without guard(), so does that mean
> > > you'd accept the first patch in the series?  
> > 
> > How can we get one without the other.. do you reckon Joe P would let us
> > add a checkpatch check to warn people against pure guard() under net/ ?  
> 
> Maybe? But I think I do want to use guard() ;-)

IIUC Willem and Stan like the construct too, so I'm fine with patches 
1 and 2. But let's not convert the exiting code just yet (leave out 3)?

Re: [PATCH 0/3] using guard/__free in networking

[Johannes Berg]

On Tue, 2024-03-26 at 17:15 -0700, Jakub Kicinski wrote:
> 
> IIUC Willem and Stan like the construct too, so I'm fine with patches 
> 1 and 2. But let's not convert the exiting code just yet (leave out 3)?

Sure, fair. It was mostly an illustration. Do you want a resend then?

johannes

Re: [PATCH 0/3] using guard/__free in networking

[Jakub Kicinski]

On Wed, 27 Mar 2024 20:24:17 +0100 Johannes Berg wrote:
> > IIUC Willem and Stan like the construct too, so I'm fine with patches 
> > 1 and 2. But let's not convert the exiting code just yet (leave out 3)?  
> 
> Sure, fair. It was mostly an illustration. Do you want a resend then?

Yes, please!

Re: [PATCH 0/3] using guard/__free in networking

[Jakub Sitnicki]

On Tue, Mar 26, 2024 at 04:33 PM +01, Johannes Berg wrote:
>> Is it also present in Rust or some such?
>
> I have no idea. I _think_ Rust actually ties the data and the locks
> together more?

That is right. Nicely explained here:

https://marabos.nl/atomics/basics.html#rusts-mutex

Re: [PATCH 0/3] using guard/__free in networking

[Johannes Berg]

On Wed, 2024-03-27 at 21:25 +0100, Jakub Sitnicki wrote:
> On Tue, Mar 26, 2024 at 04:33 PM +01, Johannes Berg wrote:
> > > Is it also present in Rust or some such?
> > 
> > I have no idea. I _think_ Rust actually ties the data and the locks
> > together more?
> 
> That is right. Nicely explained here:
> 
> https://marabos.nl/atomics/basics.html#rusts-mutex

Right.

Thinking about that, we _could_ even add support for drop_guard()?

With the below patch to cleanup.h, you can write

void my_something(my_t *my)
{
...
	named_guard(lock, mutex)(&my->mutex);
...
	if (foo)
		return -EINVAL; // automatically unlocks
...
	// no need for lock any more
	drop_guard(lock);
...
	// do other things now unlocked
}


Is that too ugly? Maybe it's less Java-like and more Rust-like and
better for Jakub ;-)

In some sense that's nicer than scoped_guard() since it doesn't require
a new scope / indentation, but maybe Peter already thought about it and
rejected it :-)


Patch follows, though maybe that should be rolled into the 'base' CLASS
definition instead of defining another "_drop" one for named_guard()?

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..31298106c28b 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -106,7 +106,27 @@ typedef _type class_##_name##_t;					\
 static inline void class_##_name##_destructor(_type *p)			\
 { _type _T = *p; _exit; }						\
 static inline _type class_##_name##_constructor(_init_args)		\
-{ _type t = _init; return t; }
+{ _type t = _init; return t; }						\
+typedef struct class_##_name##_drop##_t {				\
+	class_##_name##_t obj;						\
+	void (*destructor)(struct class_##_name##_drop##_t *);		\
+} class_##_name##_drop##_t;						\
+static inline void							\
+class_##_name##_drop##_destructor(class_##_name##_drop##_t *p)		\
+{									\
+	if (p->obj)							\
+		class_##_name##_destructor(&p->obj);			\
+	p->obj = NULL;							\
+}									\
+static inline class_##_name##_drop##_t					\
+class_##_name##_drop##_constructor(_init_args)				\
+{									\
+	class_##_name##_drop##_t t = {					\
+		.obj = _init,						\
+		.destructor = class_##_name##_drop##_destructor,	\
+	};								\
+	return t;							\
+}
 
 #define EXTEND_CLASS(_name, ext, _init, _init_args...)			\
 typedef class_##_name##_t class_##_name##ext##_t;			\
@@ -163,6 +183,11 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 #define guard(_name) \
 	CLASS(_name, __UNIQUE_ID(guard))
 
+#define named_guard(_name, _class) \
+	CLASS(_class##_drop, _name)
+
+#define drop_guard(_name) do { _name.destructor(&_name); } while (0)
+
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
 #define scoped_guard(_name, args...)					\

Re: [PATCH 0/3] using guard/__free in networking

[Johannes Berg]

On Wed, 2024-03-27 at 22:28 +0100, Johannes Berg wrote:
> 
> +typedef struct class_##_name##_drop##_t {				\
> +	class_##_name##_t obj;						\
> +	void (*destructor)(struct class_##_name##_drop##_t *);		\
> +} class_##_name##_drop##_t;						\

No, I misread the compiler output, it does output a real destructor
function to push it into a stack variable with this...

So I guess it'd have to be

void my_something(my_t *my)
{
...
	named_guard(lock, mutex)(&my->mutex);
...
	if (foo)
		return -EINVAL; // automatically unlocks
...
	// no need for lock any more
	drop_guard(lock, mutex);
...
	// do other things now unlocked
}


instead, syntax-wise.


Which obviously simplifies the changes:


diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..cf39a4a3f56f 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -163,6 +163,12 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 #define guard(_name) \
 	CLASS(_name, __UNIQUE_ID(guard))
 
+#define named_guard(_name, _class) \
+	CLASS(_class, _name)
+
+#define drop_guard(_name, _class) \
+	do { class_##_class##_destructor(&_name); _name = NULL; } while (0)
+
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
 #define scoped_guard(_name, args...)					\

Re: [PATCH 0/3] using guard/__free in networking

[Przemek Kitszel]

On 3/26/24 15:37, Jakub Kicinski wrote:
> On Tue, 26 Mar 2024 09:42:43 +0100 Johannes Berg wrote:
>> On Mon, 2024-03-25 at 19:09 -0700, Jakub Kicinski wrote:
>>> On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:
>>>> Hi,
>>>>
>>>> So I started playing with this for wifi, and overall that
>>>> does look pretty nice, but it's a bit weird if we can do
>>>>
>>>>    guard(wiphy)(&rdev->wiphy);
>>>>
>>>> or so, but still have to manually handle the RTNL in the
>>>> same code.
>>>
>>> Dunno, it locks code instead of data accesses.
>>
>> Well, I'm not sure that's a fair complaint. After all, without any more
>> compiler help, even rtnl_lock()/rtnl_unlock() _necessarily_ locks code.
>> Clearly
>>
>> 	rtnl_lock();
>> 	// something
>> 	rtnl_unlock();
>>
>> also locks the "// something" code, after all., and yeah that might be
>> doing data accesses, but it might also be a function call or a whole
>> bunch of other things?
>>
>> Or if you look at something like bpf_xdp_link_attach(), I don't think
>> you can really say that it locks only data. That doesn't even do the
>> allocation outside the lock (though I did convert that one to
>> scoped_guard because of that.)
>>
>> Or even something simple like unregister_netdev(), it just requires the
>> RTNL for some data accesses and consistency deep inside
>> unregister_netdevice(), not for any specific data accessed there.
>>
>> So yeah, this is always going to be a trade-off, but all the locking is.
>> We even make similar trade-offs manually, e.g. look at
>> bpf_xdp_link_update(), it will do the bpf_prog_put() under the RTNL
>> still, for no good reason other than simplifying the cleanup path there.
> 
> At least to me the mental model is different. 99% of the time the guard
> is covering the entire body. So now we're moving from "I'm touching X
> so I need to lock" to "This _function_ is safe to touch X".
> 
>> Anyway, I can live with it either way (unless you tell me you won't pull
>> wireless code using guard), just thought doing the wireless locking with
>> guard and the RTNL around it without it (only in a few places do we
>> still use RTNL though) looked odd.
>>
>>
>>> Forgive the comparison but it feels too much like Java to me :)
>>
>> Heh. Haven't used Java in 20 years or so...
> 
> I only did at uni, but I think they had a decorator for a method, where
> you can basically say "this method should be under lock X" and runtime
> will take that lock before entering and drop it after exit,
> appropriately. I wonder why the sudden love for this concept :S
> Is it also present in Rust or some such?

:D
There is indeed a lot of proposals around the topic lately :)

I believe that "The first half of the 6.8 merge window" [lwn] article
has brought attention to the in-kernel "scope-based resource management"
availability.

[lwn] https://lwn.net/Articles/957188/

More abstraction/sugar over __free() is cleanly needed to have code both
easier to follow and less buggy.

You made a good point to encourage scoping the locks to small blocks
instead of whole functions. And "less typing" to instantiate the lock
guard variable is one way to do that.

> 
>>> scoped_guard is fine, the guard() not so much.
>>
>> I think you can't get scoped_guard() without guard(), so does that mean
>> you'd accept the first patch in the series?
> 
> How can we get one without the other.. do you reckon Joe P would let us
> add a checkpatch check to warn people against pure guard() under net/ ?
> 
>>> Do you have a piece of code in wireless where the conversion
>>> made you go "wow, this is so much cleaner"?
>>
>> Mostly long and complex error paths. Found a double-unlock bug (in
>> iwlwifi) too, when converting some locking there.
>>
>> Doing a more broader conversion on cfg80211/mac80211 removes around 200
>> lines of unlocking, mostly error handling, code.
>>
>> Doing __free() too will probably clean up even more.
> 
> Not super convinced by that one either:
> https://lore.kernel.org/all/20240321185640.6f7f4d6b@kernel.org/
> maybe I'm too conservative..
> 

Re: [PATCH 0/3] using guard/__free in networking

[Andrew Lunn]

On Mon, Mar 25, 2024 at 07:09:57PM -0700, Jakub Kicinski wrote:
> On Mon, 25 Mar 2024 23:31:25 +0100 Johannes Berg wrote:
> > Hi,
> > 
> > So I started playing with this for wifi, and overall that
> > does look pretty nice, but it's a bit weird if we can do
> > 
> >   guard(wiphy)(&rdev->wiphy);
> > 
> > or so, but still have to manually handle the RTNL in the
> > same code.
> 
> Dunno, it locks code instead of data accesses.
> Forgive the comparison but it feels too much like Java to me :)
> scoped_guard is fine, the guard() not so much.

I tend to agree. guard() does not feel like C. scoped_guard does.

	Andrew