[RFC PATCH 0/2] tracking the references of net_device in aoe

[RFC PATCH 0/2] tracking the references of net_device in aoe

[Chun-Yi Lee]

This debug patch series is base on '[PATCH v3] aoe: fix the potential
use-after-free problem in more places' for tracking the reference count
of using net_device in aoeif. It adds a nd_pcpu_refcnt field in aoeif
structure. And two wrappers, nd_dev_hold() and nd_dev_put() are used to
call dev_hold(nd)/dev_put(nd) and maintain ifp->nd_pcpu_refcnt at the
same time.

Defined DEBUG to the top of the aoe.h can enable the tracking function.
The nd_pcpu_refcnt will be printed to debugfs:

rttavg: 249029 rttdev: 1781043
nskbpool: 0
kicked: 0
maxbcnt: 1024
ref: 0
falloc: 36
ffree: 0000000013c0033f
52540054c48e:0:16:16
        ssthresh:8
        taint:0
        r:1270
        w:8
        enp1s0:1	<-- the aoeif->nd_pcpu_refcnt is behind nd->name

The value of aoeif->nd_pcpu_refcnt will also be printed when 'rmmod aoe':

[23412.255237][ T2857] aoe: enp1s0->refcnt: 32, aoeif->nd_refcnt: 0

Using kernel dynamic debug can print more detail log but it causes extra
overhead:

echo -n 'file drivers/block/aoe/* +p' > /sys/kernel/debug/dynamic_debug/control

[ 6961.938642] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1
[ 7023.368814] aoe: aoecmd_cfg_pkts dev_hold lo->refcnt: 30
[ 7023.370530] aoe: aoecmd_cfg_pkts dev_hold enp1s0->refcnt: 32, aoeif->nd_refcnt: 2
[ 7023.372977] aoe: tx dev_put lo->refcnt: 29
[ 7023.375147] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1

Normally, after one operation of aoe, the aoeif->nd_refcnt should be
shown as '1' which means that calls of dev_hold(nd)/dev_put(nd) are
balanced. The final '1' reference of net_device will be removed when
rmmod aoe.

Chun-Yi Lee (2):
  aoe: add reference count in aoeif for tracking the using of net_device
  aoe: using wrappers instead of dev_hold/dev_put for tracking the
    references of net_device in aoeif

 drivers/block/aoe/aoe.h    | 84 ++++++++++++++++++++++++++++++++++++++
 drivers/block/aoe/aoeblk.c |  5 +++
 drivers/block/aoe/aoecmd.c | 24 +++++------
 drivers/block/aoe/aoedev.c | 23 ++++++++++-
 drivers/block/aoe/aoenet.c |  2 +-
 5 files changed, 124 insertions(+), 14 deletions(-)

-- 
2.35.3

[RFC PATCH 1/2] aoe: add reference count in aoeif for tracking the using of net_device

[Chun-Yi Lee]

This is a patch for debugging. For tracking the reference count of using
net_device in aoeif, this patch adds a nd_pcpu_refcnt field in aoeif
structure. Two wrappers, nd_dev_hold() and nd_dev_put() are used to
call dev_hold(nd)/dev_put(nd) and maintain ifp->nd_pcpu_refcnt at the
same time.

Defined DEBUG to the top of the aoe.h can enable the tracking function.
The nd_pcpu_refcnt will be printed to debugfs:

rttavg: 249029 rttdev: 1781043
nskbpool: 0
kicked: 0
maxbcnt: 1024
ref: 0
falloc: 36
ffree: 0000000013c0033f
52540054c48e:0:16:16
        ssthresh:8
        taint:0
        r:1270
        w:8
        enp1s0:1	<-- the aoeif->nd_pcpu_refcnt is behind nd->name

The value of aoeif->nd_pcpu_refcnt will also be printed when 'rmmod aoe':

[23412.255237][ T2857] aoe: enp1s0->refcnt: 32, aoeif->nd_refcnt: 0

Using kernel dynamic debug can print more detail log but it causes extra
overhead:

echo -n 'file drivers/block/aoe/* +p' > /sys/kernel/debug/dynamic_debug/control

[ 6961.938642] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1
[ 7023.368814] aoe: aoecmd_cfg_pkts dev_hold lo->refcnt: 30
[ 7023.370530] aoe: aoecmd_cfg_pkts dev_hold enp1s0->refcnt: 32, aoeif->nd_refcnt: 2
[ 7023.372977] aoe: tx dev_put lo->refcnt: 29
[ 7023.375147] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1

Normally, after one operation of aoe, the aoeif->nd_refcnt should be
shown as '1' which means that calls of dev_hold(nd)/dev_put(nd) are
balanced. The final '1' reference of net_device will be removed when
rmmod aoe.

Signed-off-by: Chun-Yi Lee <jlee@suse.com>
---
 drivers/block/aoe/aoe.h    | 84 ++++++++++++++++++++++++++++++++++++++
 drivers/block/aoe/aoeblk.c |  5 +++
 drivers/block/aoe/aoedev.c | 20 +++++++++
 3 files changed, 109 insertions(+)

diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
index 749ae1246f4c..a6d954562794 100644
--- a/drivers/block/aoe/aoe.h
+++ b/drivers/block/aoe/aoe.h
@@ -1,5 +1,6 @@
 /* Copyright (c) 2013 Coraid, Inc.  See COPYING for GPL terms. */
 #include <linux/blk-mq.h>
+#include <linux/netdevice.h>
 
 #define VERSION "85"
 #define AOE_MAJOR 152
@@ -133,6 +134,9 @@ struct aoeif {
 	struct net_device *nd;
 	ulong lost;
 	int bcnt;
+#ifdef DEBUG
+	int __percpu *nd_pcpu_refcnt;
+#endif
 };
 
 struct aoetgt {
@@ -238,6 +242,7 @@ void aoedev_downdev(struct aoedev *d);
 int aoedev_flush(const char __user *str, size_t size);
 void aoe_failbuf(struct aoedev *, struct buf *);
 void aoedev_put(struct aoedev *);
+struct aoeif *get_aoeif(struct net_device *nd);
 
 int aoenet_init(void);
 void aoenet_exit(void);
@@ -246,3 +251,82 @@ int is_aoe_netif(struct net_device *ifp);
 int set_aoe_iflist(const char __user *str, size_t size);
 
 extern struct workqueue_struct *aoe_wq;
+
+#ifdef DEBUG
+static inline int aoeif_nd_refcnt_read(const struct aoeif *ifp)
+{
+       int i, refcnt = 0;
+
+       for_each_possible_cpu(i)
+               refcnt += *per_cpu_ptr(ifp->nd_pcpu_refcnt, i);
+       return refcnt;
+}
+
+static inline void aoeif_nd_refcnt_free(struct aoeif *ifp)
+{
+	int i;
+
+	if(!ifp)
+		return;
+	if (ifp->nd)
+		pr_info("aoe: %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
+			ifp->nd->name, netdev_refcnt_read(ifp->nd),
+			aoeif_nd_refcnt_read(ifp));
+	else
+		pr_info("aoe: aoeif->nd_refcnt: %d\n", aoeif_nd_refcnt_read(ifp));
+
+	for_each_possible_cpu(i)
+		*per_cpu_ptr(ifp->nd_pcpu_refcnt, i) = 0;
+	free_percpu(ifp->nd_pcpu_refcnt);
+	ifp->nd_pcpu_refcnt = NULL;
+}
+
+/* ifi aoeif input, nb be set to aoeif or in the future will be set */
+static inline void __nd_dev_hold(const char *str, struct net_device *nd, struct aoeif *ifi)
+{
+	struct aoeif *ifp;
+
+	if (!nd)
+		return;
+	dev_hold(nd);
+	ifp = ifi? ifi:get_aoeif(nd);
+	if (ifp) {
+		this_cpu_inc(*ifp->nd_pcpu_refcnt);
+		pr_debug("aoe: %s dev_hold %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
+			 str, nd->name, netdev_refcnt_read(nd),
+			 aoeif_nd_refcnt_read(ifp));
+	} else
+		pr_debug("aoe: %s dev_hold %s->refcnt: %d\n",
+			 str, nd->name, netdev_refcnt_read(nd));
+}
+#define nd_dev_hold(msg, ifi) __nd_dev_hold(__FUNCTION__, (msg), (ifi))
+
+static inline void __nd_dev_put(const char *str, struct net_device *nd, struct aoeif *ifi)
+{
+	struct aoeif *ifp;
+
+	if (!nd)
+		return;
+	dev_put(nd);
+	ifp = ifi? ifi:get_aoeif(nd);
+	if (ifp) {
+		this_cpu_dec(*ifp->nd_pcpu_refcnt);
+		pr_debug("aoe: %s dev_put %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
+			 str, nd->name, netdev_refcnt_read(nd),
+			 aoeif_nd_refcnt_read(ifp));
+	} else
+		pr_debug("aoe: %s dev_put %s->refcnt: %d\n",
+			 str, nd->name, netdev_refcnt_read(nd));
+}
+#define nd_dev_put(msg, ifi) __nd_dev_put(__FUNCTION__, (msg), (ifi))
+#else
+static inline void nd_dev_put(struct net_device *nd, struct aoeif *ifi)
+{
+	dev_hold(nd);
+}
+static inline void nd_dev_hold(struct net_device *nd, struct aoeif *ifi)
+{
+       dev_put(nd);
+}
+static inline void aoeif_nd_refcnt_free(const struct aoeif *ifp) {}
+#endif // DEBUG
diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
index 2028795ec61c..19d62ccca1e9 100644
--- a/drivers/block/aoe/aoeblk.c
+++ b/drivers/block/aoe/aoeblk.c
@@ -142,7 +142,12 @@ static int aoe_debugfs_show(struct seq_file *s, void *ignored)
 		ifp = (*t)->ifs;
 		ife = ifp + ARRAY_SIZE((*t)->ifs);
 		for (; ifp->nd && ifp < ife; ifp++) {
+#ifdef DEBUG
+			seq_printf(s, "%c%s:%d", c, ifp->nd->name,
+					aoeif_nd_refcnt_read(ifp));
+#else
 			seq_printf(s, "%c%s", c, ifp->nd->name);
+#endif
 			c = ',';
 		}
 		seq_puts(s, "\n");
diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
index 3523dd82d7a0..9781488b286b 100644
--- a/drivers/block/aoe/aoedev.c
+++ b/drivers/block/aoe/aoedev.c
@@ -529,3 +529,23 @@ aoedev_init(void)
 {
 	return 0;
 }
+
+struct aoeif *
+get_aoeif(struct net_device *nd)
+{
+	struct aoedev *d;
+	struct aoetgt *t, **tt, **te;
+	struct aoeif *ifp;
+
+	for (d=devlist; d; d=d->next) {
+		tt = d->targets;
+		te = tt + d->ntargets;
+		for (; tt < te && (t = *tt); tt++) {
+			for (ifp = t->ifs; ifp < &t->ifs[NAOEIFS]; ++ifp) {
+				if (ifp->nd && (ifp->nd == nd))
+					return ifp;
+			}
+		}
+	}
+	return NULL;
+}
-- 
2.35.3

[RFC PATCH 2/2] aoe: using wrappers instead of dev_hold/dev_put for tracking the references of net_device in aoeif

[Chun-Yi Lee]

Signed-off-by: Chun-Yi Lee <jlee@suse.com>
---
 drivers/block/aoe/aoecmd.c | 24 ++++++++++++------------
 drivers/block/aoe/aoedev.c |  3 ++-
 drivers/block/aoe/aoenet.c |  2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
index d1f4ddc57645..2bae364fc5ef 100644
--- a/drivers/block/aoe/aoecmd.c
+++ b/drivers/block/aoe/aoecmd.c
@@ -361,7 +361,7 @@ ata_rw_frameinit(struct frame *f)
 	}
 
 	ah->cmdstat = ATA_CMD_PIO_READ | writebit | extbit;
-	dev_hold(t->ifp->nd);
+	nd_dev_hold(t->ifp->nd, t->ifp);
 	skb->dev = t->ifp->nd;
 }
 
@@ -403,7 +403,7 @@ aoecmd_ata_rw(struct aoedev *d)
 		__skb_queue_tail(&queue, skb);
 		aoenet_xmit(&queue);
 	} else {
-		dev_put(f->t->ifp->nd);
+		nd_dev_put(f->t->ifp->nd, f->t->ifp);
 	}
 	return 1;
 }
@@ -421,16 +421,16 @@ aoecmd_cfg_pkts(ushort aoemajor, unsigned char aoeminor, struct sk_buff_head *qu
 
 	rcu_read_lock();
 	for_each_netdev_rcu(&init_net, ifp) {
-		dev_hold(ifp);
+		nd_dev_hold(ifp, NULL);
 		if (!is_aoe_netif(ifp)) {
-			dev_put(ifp);
+			nd_dev_put(ifp, NULL);
 			continue;
 		}
 
 		skb = new_skb(sizeof *h + sizeof *ch);
 		if (skb == NULL) {
 			printk(KERN_INFO "aoe: skb alloc failure\n");
-			dev_put(ifp);
+			nd_dev_put(ifp, NULL);
 			continue;
 		}
 		skb_put(skb, sizeof *h + sizeof *ch);
@@ -486,11 +486,11 @@ resend(struct aoedev *d, struct frame *f)
 	memcpy(h->dst, t->addr, sizeof h->dst);
 	memcpy(h->src, t->ifp->nd->dev_addr, sizeof h->src);
 
-	dev_hold(t->ifp->nd);
+	nd_dev_hold(t->ifp->nd, t->ifp);
 	skb->dev = t->ifp->nd;
 	skb = skb_clone(skb, GFP_ATOMIC);
 	if (skb == NULL) {
-		dev_put(t->ifp->nd);
+		nd_dev_put(t->ifp->nd, t->ifp);
 		return;
 	}
 	f->sent = ktime_get();
@@ -552,7 +552,7 @@ ejectif(struct aoetgt *t, struct aoeif *ifp)
 	n = (e - ifp) * sizeof *ifp;
 	memmove(ifp, ifp+1, n);
 	e->nd = NULL;
-	dev_put(nd);
+	nd_dev_put(nd, NULL);
 }
 
 static struct frame *
@@ -624,7 +624,7 @@ probe(struct aoetgt *t)
 		__skb_queue_tail(&queue, skb);
 		aoenet_xmit(&queue);
 	} else {
-		dev_put(f->t->ifp->nd);
+		nd_dev_put(f->t->ifp->nd, f->t->ifp);
 	}
 }
 
@@ -1403,7 +1403,7 @@ aoecmd_ata_id(struct aoedev *d)
 	ah->cmdstat = ATA_CMD_ID_ATA;
 	ah->lba3 = 0xa0;
 
-	dev_hold(t->ifp->nd);
+	nd_dev_hold(t->ifp->nd, t->ifp);
 	skb->dev = t->ifp->nd;
 
 	d->rttavg = RTTAVG_INIT;
@@ -1414,7 +1414,7 @@ aoecmd_ata_id(struct aoedev *d)
 	if (skb)
 		f->sent = ktime_get();
 	else
-		dev_put(t->ifp->nd);
+		nd_dev_put(t->ifp->nd, t->ifp);
 
 	return skb;
 }
@@ -1514,7 +1514,7 @@ setifbcnt(struct aoetgt *t, struct net_device *nd, int bcnt)
 			pr_err("aoe: device setifbcnt failure; too many interfaces.\n");
 			return;
 		}
-		dev_hold(nd);
+		nd_dev_hold(nd, p);
 		p->nd = nd;
 		p->bcnt = bcnt;
 	}
diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
index 9781488b286b..48c936dbb9e5 100644
--- a/drivers/block/aoe/aoedev.c
+++ b/drivers/block/aoe/aoedev.c
@@ -504,7 +504,8 @@ freetgt(struct aoedev *d, struct aoetgt *t)
 	for (ifp = t->ifs; ifp < &t->ifs[NAOEIFS]; ++ifp) {
 		if (!ifp->nd)
 			break;
-		dev_put(ifp->nd);
+		nd_dev_put(ifp->nd, ifp);
+		aoeif_nd_refcnt_free(ifp);
 	}
 
 	head = &t->ffree;
diff --git a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
index 923a134fd766..3565042b567f 100644
--- a/drivers/block/aoe/aoenet.c
+++ b/drivers/block/aoe/aoenet.c
@@ -63,7 +63,7 @@ tx(int id) __must_hold(&txlock)
 			pr_warn("aoe: packet could not be sent on %s.  %s\n",
 				ifp ? ifp->name : "netif",
 				"consider increasing tx_queue_len");
-		dev_put(ifp);
+		nd_dev_put(ifp, NULL);
 		spin_lock_irq(&txlock);
 	}
 	return 0;
-- 
2.35.3

Re: [RFC PATCH 1/2] aoe: add reference count in aoeif for tracking the using of net_device

[Damien Le Moal]

On 10/2/24 1:06 PM, Chun-Yi Lee wrote:
> This is a patch for debugging. For tracking the reference count of using
> net_device in aoeif, this patch adds a nd_pcpu_refcnt field in aoeif
> structure. Two wrappers, nd_dev_hold() and nd_dev_put() are used to
> call dev_hold(nd)/dev_put(nd) and maintain ifp->nd_pcpu_refcnt at the
> same time.
> 
> Defined DEBUG to the top of the aoe.h can enable the tracking function.
> The nd_pcpu_refcnt will be printed to debugfs:

Why not make that a config option ? That would avoid having to edit the code to
enable debugging...

> 
> rttavg: 249029 rttdev: 1781043
> nskbpool: 0
> kicked: 0
> maxbcnt: 1024
> ref: 0
> falloc: 36
> ffree: 0000000013c0033f
> 52540054c48e:0:16:16
>         ssthresh:8
>         taint:0
>         r:1270
>         w:8
>         enp1s0:1	<-- the aoeif->nd_pcpu_refcnt is behind nd->name
> 
> The value of aoeif->nd_pcpu_refcnt will also be printed when 'rmmod aoe':
> 
> [23412.255237][ T2857] aoe: enp1s0->refcnt: 32, aoeif->nd_refcnt: 0
> 
> Using kernel dynamic debug can print more detail log but it causes extra
> overhead:
> 
> echo -n 'file drivers/block/aoe/* +p' > /sys/kernel/debug/dynamic_debug/control
> 
> [ 6961.938642] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1
> [ 7023.368814] aoe: aoecmd_cfg_pkts dev_hold lo->refcnt: 30
> [ 7023.370530] aoe: aoecmd_cfg_pkts dev_hold enp1s0->refcnt: 32, aoeif->nd_refcnt: 2
> [ 7023.372977] aoe: tx dev_put lo->refcnt: 29
> [ 7023.375147] aoe: tx dev_put enp1s0->refcnt: 31, aoeif->nd_refcnt: 1
> 
> Normally, after one operation of aoe, the aoeif->nd_refcnt should be
> shown as '1' which means that calls of dev_hold(nd)/dev_put(nd) are
> balanced. The final '1' reference of net_device will be removed when
> rmmod aoe.
> 
> Signed-off-by: Chun-Yi Lee <jlee@suse.com>
> ---
>  drivers/block/aoe/aoe.h    | 84 ++++++++++++++++++++++++++++++++++++++
>  drivers/block/aoe/aoeblk.c |  5 +++
>  drivers/block/aoe/aoedev.c | 20 +++++++++
>  3 files changed, 109 insertions(+)
> 
> diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
> index 749ae1246f4c..a6d954562794 100644
> --- a/drivers/block/aoe/aoe.h
> +++ b/drivers/block/aoe/aoe.h
> @@ -1,5 +1,6 @@
>  /* Copyright (c) 2013 Coraid, Inc.  See COPYING for GPL terms. */
>  #include <linux/blk-mq.h>
> +#include <linux/netdevice.h>
>  
>  #define VERSION "85"
>  #define AOE_MAJOR 152
> @@ -133,6 +134,9 @@ struct aoeif {
>  	struct net_device *nd;
>  	ulong lost;
>  	int bcnt;
> +#ifdef DEBUG
> +	int __percpu *nd_pcpu_refcnt;
> +#endif
>  };
>  
>  struct aoetgt {
> @@ -238,6 +242,7 @@ void aoedev_downdev(struct aoedev *d);
>  int aoedev_flush(const char __user *str, size_t size);
>  void aoe_failbuf(struct aoedev *, struct buf *);
>  void aoedev_put(struct aoedev *);
> +struct aoeif *get_aoeif(struct net_device *nd);
>  
>  int aoenet_init(void);
>  void aoenet_exit(void);
> @@ -246,3 +251,82 @@ int is_aoe_netif(struct net_device *ifp);
>  int set_aoe_iflist(const char __user *str, size_t size);
>  
>  extern struct workqueue_struct *aoe_wq;
> +
> +#ifdef DEBUG
> +static inline int aoeif_nd_refcnt_read(const struct aoeif *ifp)
> +{
> +       int i, refcnt = 0;
> +
> +       for_each_possible_cpu(i)
> +               refcnt += *per_cpu_ptr(ifp->nd_pcpu_refcnt, i);
> +       return refcnt;
> +}
> +
> +static inline void aoeif_nd_refcnt_free(struct aoeif *ifp)
> +{
> +	int i;
> +
> +	if(!ifp)
> +		return;
> +	if (ifp->nd)
> +		pr_info("aoe: %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
> +			ifp->nd->name, netdev_refcnt_read(ifp->nd),
> +			aoeif_nd_refcnt_read(ifp));
> +	else
> +		pr_info("aoe: aoeif->nd_refcnt: %d\n", aoeif_nd_refcnt_read(ifp));
> +
> +	for_each_possible_cpu(i)
> +		*per_cpu_ptr(ifp->nd_pcpu_refcnt, i) = 0;
> +	free_percpu(ifp->nd_pcpu_refcnt);
> +	ifp->nd_pcpu_refcnt = NULL;
> +}
> +
> +/* ifi aoeif input, nb be set to aoeif or in the future will be set */
> +static inline void __nd_dev_hold(const char *str, struct net_device *nd, struct aoeif *ifi)
> +{
> +	struct aoeif *ifp;
> +
> +	if (!nd)
> +		return;
> +	dev_hold(nd);
> +	ifp = ifi? ifi:get_aoeif(nd);
> +	if (ifp) {
> +		this_cpu_inc(*ifp->nd_pcpu_refcnt);
> +		pr_debug("aoe: %s dev_hold %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
> +			 str, nd->name, netdev_refcnt_read(nd),
> +			 aoeif_nd_refcnt_read(ifp));
> +	} else
> +		pr_debug("aoe: %s dev_hold %s->refcnt: %d\n",
> +			 str, nd->name, netdev_refcnt_read(nd));

Missing curly brackets around the else statement.

> +}
> +#define nd_dev_hold(msg, ifi) __nd_dev_hold(__FUNCTION__, (msg), (ifi))
> +
> +static inline void __nd_dev_put(const char *str, struct net_device *nd, struct aoeif *ifi)
> +{
> +	struct aoeif *ifp;
> +
> +	if (!nd)
> +		return;
> +	dev_put(nd);
> +	ifp = ifi? ifi:get_aoeif(nd);
> +	if (ifp) {
> +		this_cpu_dec(*ifp->nd_pcpu_refcnt);
> +		pr_debug("aoe: %s dev_put %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
> +			 str, nd->name, netdev_refcnt_read(nd),
> +			 aoeif_nd_refcnt_read(ifp));
> +	} else
> +		pr_debug("aoe: %s dev_put %s->refcnt: %d\n",
> +			 str, nd->name, netdev_refcnt_read(nd));

Same here.

> +}
> +#define nd_dev_put(msg, ifi) __nd_dev_put(__FUNCTION__, (msg), (ifi))
> +#else
> +static inline void nd_dev_put(struct net_device *nd, struct aoeif *ifi)
> +{
> +	dev_hold(nd);
> +}
> +static inline void nd_dev_hold(struct net_device *nd, struct aoeif *ifi)
> +{
> +       dev_put(nd);
> +}
> +static inline void aoeif_nd_refcnt_free(const struct aoeif *ifp) {}
> +#endif // DEBUG
> diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
> index 2028795ec61c..19d62ccca1e9 100644
> --- a/drivers/block/aoe/aoeblk.c
> +++ b/drivers/block/aoe/aoeblk.c
> @@ -142,7 +142,12 @@ static int aoe_debugfs_show(struct seq_file *s, void *ignored)
>  		ifp = (*t)->ifs;
>  		ife = ifp + ARRAY_SIZE((*t)->ifs);
>  		for (; ifp->nd && ifp < ife; ifp++) {
> +#ifdef DEBUG
> +			seq_printf(s, "%c%s:%d", c, ifp->nd->name,
> +					aoeif_nd_refcnt_read(ifp));

I personnally find it better looking to align the arguments instead of adding a
random tab...

> +#else
>  			seq_printf(s, "%c%s", c, ifp->nd->name);
> +#endif
>  			c = ',';
>  		}
>  		seq_puts(s, "\n");
> diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
> index 3523dd82d7a0..9781488b286b 100644
> --- a/drivers/block/aoe/aoedev.c
> +++ b/drivers/block/aoe/aoedev.c
> @@ -529,3 +529,23 @@ aoedev_init(void)
>  {
>  	return 0;
>  }
> +
> +struct aoeif *
> +get_aoeif(struct net_device *nd)

Why the line split after "*" ?

> +{
> +	struct aoedev *d;
> +	struct aoetgt *t, **tt, **te;
> +	struct aoeif *ifp;
> +
> +	for (d=devlist; d; d=d->next) {
> +		tt = d->targets;
> +		te = tt + d->ntargets;
> +		for (; tt < te && (t = *tt); tt++) {
> +			for (ifp = t->ifs; ifp < &t->ifs[NAOEIFS]; ++ifp) {
> +				if (ifp->nd && (ifp->nd == nd))
> +					return ifp;
> +			}
> +		}
> +	}
> +	return NULL;
> +}


-- 
Damien Le Moal
Western Digital Research

Re: [RFC PATCH 2/2] aoe: using wrappers instead of dev_hold/dev_put for tracking the references of net_device in aoeif

[Damien Le Moal]

On 10/2/24 1:06 PM, Chun-Yi Lee wrote:
> Signed-off-by: Chun-Yi Lee <jlee@suse.com>

The wrappers where introduced in patch 1 without any user. So it seems that
this patch should be squashed together with patch 1.

> ---
>  drivers/block/aoe/aoecmd.c | 24 ++++++++++++------------
>  drivers/block/aoe/aoedev.c |  3 ++-
>  drivers/block/aoe/aoenet.c |  2 +-
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c
> index d1f4ddc57645..2bae364fc5ef 100644
> --- a/drivers/block/aoe/aoecmd.c
> +++ b/drivers/block/aoe/aoecmd.c
> @@ -361,7 +361,7 @@ ata_rw_frameinit(struct frame *f)
>  	}
>  
>  	ah->cmdstat = ATA_CMD_PIO_READ | writebit | extbit;
> -	dev_hold(t->ifp->nd);
> +	nd_dev_hold(t->ifp->nd, t->ifp);
>  	skb->dev = t->ifp->nd;
>  }
>  
> @@ -403,7 +403,7 @@ aoecmd_ata_rw(struct aoedev *d)
>  		__skb_queue_tail(&queue, skb);
>  		aoenet_xmit(&queue);
>  	} else {
> -		dev_put(f->t->ifp->nd);
> +		nd_dev_put(f->t->ifp->nd, f->t->ifp);
>  	}
>  	return 1;
>  }
> @@ -421,16 +421,16 @@ aoecmd_cfg_pkts(ushort aoemajor, unsigned char aoeminor, struct sk_buff_head *qu
>  
>  	rcu_read_lock();
>  	for_each_netdev_rcu(&init_net, ifp) {
> -		dev_hold(ifp);
> +		nd_dev_hold(ifp, NULL);
>  		if (!is_aoe_netif(ifp)) {
> -			dev_put(ifp);
> +			nd_dev_put(ifp, NULL);
>  			continue;
>  		}
>  
>  		skb = new_skb(sizeof *h + sizeof *ch);
>  		if (skb == NULL) {
>  			printk(KERN_INFO "aoe: skb alloc failure\n");
> -			dev_put(ifp);
> +			nd_dev_put(ifp, NULL);
>  			continue;
>  		}
>  		skb_put(skb, sizeof *h + sizeof *ch);
> @@ -486,11 +486,11 @@ resend(struct aoedev *d, struct frame *f)
>  	memcpy(h->dst, t->addr, sizeof h->dst);
>  	memcpy(h->src, t->ifp->nd->dev_addr, sizeof h->src);
>  
> -	dev_hold(t->ifp->nd);
> +	nd_dev_hold(t->ifp->nd, t->ifp);
>  	skb->dev = t->ifp->nd;
>  	skb = skb_clone(skb, GFP_ATOMIC);
>  	if (skb == NULL) {
> -		dev_put(t->ifp->nd);
> +		nd_dev_put(t->ifp->nd, t->ifp);
>  		return;
>  	}
>  	f->sent = ktime_get();
> @@ -552,7 +552,7 @@ ejectif(struct aoetgt *t, struct aoeif *ifp)
>  	n = (e - ifp) * sizeof *ifp;
>  	memmove(ifp, ifp+1, n);
>  	e->nd = NULL;
> -	dev_put(nd);
> +	nd_dev_put(nd, NULL);
>  }
>  
>  static struct frame *
> @@ -624,7 +624,7 @@ probe(struct aoetgt *t)
>  		__skb_queue_tail(&queue, skb);
>  		aoenet_xmit(&queue);
>  	} else {
> -		dev_put(f->t->ifp->nd);
> +		nd_dev_put(f->t->ifp->nd, f->t->ifp);
>  	}
>  }
>  
> @@ -1403,7 +1403,7 @@ aoecmd_ata_id(struct aoedev *d)
>  	ah->cmdstat = ATA_CMD_ID_ATA;
>  	ah->lba3 = 0xa0;
>  
> -	dev_hold(t->ifp->nd);
> +	nd_dev_hold(t->ifp->nd, t->ifp);
>  	skb->dev = t->ifp->nd;
>  
>  	d->rttavg = RTTAVG_INIT;
> @@ -1414,7 +1414,7 @@ aoecmd_ata_id(struct aoedev *d)
>  	if (skb)
>  		f->sent = ktime_get();
>  	else
> -		dev_put(t->ifp->nd);
> +		nd_dev_put(t->ifp->nd, t->ifp);
>  
>  	return skb;
>  }
> @@ -1514,7 +1514,7 @@ setifbcnt(struct aoetgt *t, struct net_device *nd, int bcnt)
>  			pr_err("aoe: device setifbcnt failure; too many interfaces.\n");
>  			return;
>  		}
> -		dev_hold(nd);
> +		nd_dev_hold(nd, p);
>  		p->nd = nd;
>  		p->bcnt = bcnt;
>  	}
> diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
> index 9781488b286b..48c936dbb9e5 100644
> --- a/drivers/block/aoe/aoedev.c
> +++ b/drivers/block/aoe/aoedev.c
> @@ -504,7 +504,8 @@ freetgt(struct aoedev *d, struct aoetgt *t)
>  	for (ifp = t->ifs; ifp < &t->ifs[NAOEIFS]; ++ifp) {
>  		if (!ifp->nd)
>  			break;
> -		dev_put(ifp->nd);
> +		nd_dev_put(ifp->nd, ifp);
> +		aoeif_nd_refcnt_free(ifp);
>  	}
>  
>  	head = &t->ffree;
> diff --git a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
> index 923a134fd766..3565042b567f 100644
> --- a/drivers/block/aoe/aoenet.c
> +++ b/drivers/block/aoe/aoenet.c
> @@ -63,7 +63,7 @@ tx(int id) __must_hold(&txlock)
>  			pr_warn("aoe: packet could not be sent on %s.  %s\n",
>  				ifp ? ifp->name : "netif",
>  				"consider increasing tx_queue_len");
> -		dev_put(ifp);
> +		nd_dev_put(ifp, NULL);
>  		spin_lock_irq(&txlock);
>  	}
>  	return 0;


-- 
Damien Le Moal
Western Digital Research

Re: [RFC PATCH 2/2] aoe: using wrappers instead of dev_hold/dev_put for tracking the references of net_device in aoeif

[Greg KH]

On Wed, Oct 02, 2024 at 12:06:16PM +0800, Chun-Yi Lee wrote:
> Signed-off-by: Chun-Yi Lee <jlee@suse.com>

For obvious reasons, we can't take patches without any changelog
comments, nor really review them either :(

thanks,

greg k-h

Re: [RFC PATCH 1/2] aoe: add reference count in aoeif for tracking the using of net_device

[Jens Axboe]

On 10/1/24 10:06 PM, Chun-Yi Lee wrote:
> This is a patch for debugging. For tracking the reference count of using
> net_device in aoeif, this patch adds a nd_pcpu_refcnt field in aoeif
> structure. Two wrappers, nd_dev_hold() and nd_dev_put() are used to
> call dev_hold(nd)/dev_put(nd) and maintain ifp->nd_pcpu_refcnt at the
> same time.

There's no parallel universe in which using a percpu reference over just
a refcount_t for something like aoe is warranted.

-- 
Jens Axboe

Re: [RFC PATCH 1/2] aoe: add reference count in aoeif for tracking the using of net_device

[joeyli]

Hi Damien, 

Thanks for your review, first!

On Wed, Oct 02, 2024 at 02:35:33PM +0900, Damien Le Moal wrote:
> On 10/2/24 1:06 PM, Chun-Yi Lee wrote:
> > This is a patch for debugging. For tracking the reference count of using
> > net_device in aoeif, this patch adds a nd_pcpu_refcnt field in aoeif
> > structure. Two wrappers, nd_dev_hold() and nd_dev_put() are used to
> > call dev_hold(nd)/dev_put(nd) and maintain ifp->nd_pcpu_refcnt at the
> > same time.
> > 
> > Defined DEBUG to the top of the aoe.h can enable the tracking function.
> > The nd_pcpu_refcnt will be printed to debugfs:
> 
> Why not make that a config option ? That would avoid having to edit the code to
> enable debugging...
>

This debug patch is only for tracking the reference count of net_device but
no other debugging feature. I don't want to add one more config option for
this small feature. On the other hand, the tracking requirment of reference
count is more for developer but not for administrator. That's why I did not
add a new option.
 
> > 
> > rttavg: 249029 rttdev: 1781043
> > nskbpool: 0
> > kicked: 0
> > maxbcnt: 1024
> > ref: 0
> > falloc: 36
> > ffree: 0000000013c0033f
> > 52540054c48e:0:16:16
> >         ssthresh:8
> >         taint:0
> >         r:1270
> >         w:8
> >         enp1s0:1	<-- the aoeif->nd_pcpu_refcnt is behind nd->name
> > 
[...snip]
> > +	ifp = ifi? ifi:get_aoeif(nd);
> > +	if (ifp) {
> > +		this_cpu_inc(*ifp->nd_pcpu_refcnt);
> > +		pr_debug("aoe: %s dev_hold %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
> > +			 str, nd->name, netdev_refcnt_read(nd),
> > +			 aoeif_nd_refcnt_read(ifp));
> > +	} else
> > +		pr_debug("aoe: %s dev_hold %s->refcnt: %d\n",
> > +			 str, nd->name, netdev_refcnt_read(nd));
> 
> Missing curly brackets around the else statement.
>

Thanks for your reminder! I will add it in next version.
 
> > +}
> > +#define nd_dev_hold(msg, ifi) __nd_dev_hold(__FUNCTION__, (msg), (ifi))
> > +
> > +static inline void __nd_dev_put(const char *str, struct net_device *nd, struct aoeif *ifi)
> > +{
> > +	struct aoeif *ifp;
> > +
> > +	if (!nd)
> > +		return;
> > +	dev_put(nd);
> > +	ifp = ifi? ifi:get_aoeif(nd);
> > +	if (ifp) {
> > +		this_cpu_dec(*ifp->nd_pcpu_refcnt);
> > +		pr_debug("aoe: %s dev_put %s->refcnt: %d, aoeif->nd_refcnt: %d\n",
> > +			 str, nd->name, netdev_refcnt_read(nd),
> > +			 aoeif_nd_refcnt_read(ifp));
> > +	} else
> > +		pr_debug("aoe: %s dev_put %s->refcnt: %d\n",
> > +			 str, nd->name, netdev_refcnt_read(nd));
> 
> Same here.
>

Thanks! I will add it.
 
> > +}
> > +#define nd_dev_put(msg, ifi) __nd_dev_put(__FUNCTION__, (msg), (ifi))
> > +#else
> > +static inline void nd_dev_put(struct net_device *nd, struct aoeif *ifi)
> > +{
> > +	dev_hold(nd);
> > +}
> > +static inline void nd_dev_hold(struct net_device *nd, struct aoeif *ifi)
> > +{
> > +       dev_put(nd);
> > +}
> > +static inline void aoeif_nd_refcnt_free(const struct aoeif *ifp) {}
> > +#endif // DEBUG
> > diff --git a/drivers/block/aoe/aoeblk.c b/drivers/block/aoe/aoeblk.c
> > index 2028795ec61c..19d62ccca1e9 100644
> > --- a/drivers/block/aoe/aoeblk.c
> > +++ b/drivers/block/aoe/aoeblk.c
> > @@ -142,7 +142,12 @@ static int aoe_debugfs_show(struct seq_file *s, void *ignored)
> >  		ifp = (*t)->ifs;
> >  		ife = ifp + ARRAY_SIZE((*t)->ifs);
> >  		for (; ifp->nd && ifp < ife; ifp++) {
> > +#ifdef DEBUG
> > +			seq_printf(s, "%c%s:%d", c, ifp->nd->name,
> > +					aoeif_nd_refcnt_read(ifp));
> 
> I personnally find it better looking to align the arguments instead of adding a
> random tab...
>

Thanks! I will modify the second line to align with the first argument.
 
> > +#else
> >  			seq_printf(s, "%c%s", c, ifp->nd->name);
> > +#endif
> >  			c = ',';
> >  		}
> >  		seq_puts(s, "\n");
> > diff --git a/drivers/block/aoe/aoedev.c b/drivers/block/aoe/aoedev.c
> > index 3523dd82d7a0..9781488b286b 100644
> > --- a/drivers/block/aoe/aoedev.c
> > +++ b/drivers/block/aoe/aoedev.c
> > @@ -529,3 +529,23 @@ aoedev_init(void)
> >  {
> >  	return 0;
> >  }
> > +
> > +struct aoeif *
> > +get_aoeif(struct net_device *nd)
> 
> Why the line split after "*" ?
> 

I followed the same coding style in aoedev.c:

/* find it or allocate it */
struct aoedev *
aoedev_by_aoeaddr(ulong maj, int min, int do_alloc)
{
        struct aoedev *d;
...

If kernel coding style does not specify this. I prefer follow the original
style in the same driver.

Let me know if I missed anything, please. Then I will change the style. 


Thanks a lot!
Joey Lee

Re: [RFC PATCH 2/2] aoe: using wrappers instead of dev_hold/dev_put for tracking the references of net_device in aoeif

[joeyli]

Hi Damien,

On Wed, Oct 02, 2024 at 02:37:12PM +0900, Damien Le Moal wrote:
> On 10/2/24 1:06 PM, Chun-Yi Lee wrote:
> > Signed-off-by: Chun-Yi Lee <jlee@suse.com>
> 
> The wrappers where introduced in patch 1 without any user. So it seems that
> this patch should be squashed together with patch 1.
>

I separated this two patches because the second one is base on another patch
'[PATCH v3] aoe: fix the potential use-after-free problem in more places'. 

Now that patch be accepted by Jens:
https://www.spinics.net/lists/kernel/msg5384787.html

According this, I will merge this patch with the 'PATCH 1' in next version.

Thanks for your review and suggestion!

Joey Lee

Re: [RFC PATCH 2/2] aoe: using wrappers instead of dev_hold/dev_put for tracking the references of net_device in aoeif

[joeyli]

Hi Greg, 

On Wed, Oct 02, 2024 at 08:30:49AM +0200, Greg KH wrote:
> On Wed, Oct 02, 2024 at 12:06:16PM +0800, Chun-Yi Lee wrote:
> > Signed-off-by: Chun-Yi Lee <jlee@suse.com>
> 
> For obvious reasons, we can't take patches without any changelog
> comments, nor really review them either :(
>

I will merge the 'PATCH 2' with 'PATCH 1' in next version.

Thanks for your review!

Joey Lee

Re: [RFC PATCH 1/2] aoe: add reference count in aoeif for tracking the using of net_device

[joeyli]

Hi Jens,

On Wed, Oct 02, 2024 at 12:38:30PM -0600, Jens Axboe wrote:
> On 10/1/24 10:06 PM, Chun-Yi Lee wrote:
> > This is a patch for debugging. For tracking the reference count of using
> > net_device in aoeif, this patch adds a nd_pcpu_refcnt field in aoeif
> > structure. Two wrappers, nd_dev_hold() and nd_dev_put() are used to
> > call dev_hold(nd)/dev_put(nd) and maintain ifp->nd_pcpu_refcnt at the
> > same time.
> 
> There's no parallel universe in which using a percpu reference over just
> a refcount_t for something like aoe is warranted.
>

Thanks for your review! I will use refcount_t in next version.

Joey Lee