All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hillf Danton <hdanton@sina.com>
To: Zhang Cen <rollkingzzc@gmail.com>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
	linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
	zerocling0077@gmail.com
Subject: Re: [PATCH] Bluetooth: 6lowpan: Defer peer channel release until RCU cleanup
Date: Sun, 10 May 2026 09:43:56 +0800	[thread overview]
Message-ID: <20260510014359.472-1-hdanton@sina.com> (raw)
In-Reply-To: <20260509173745.413473-1-rollkingzzc@gmail.com>

On Sun, 10 May 2026 01:37:45 +0800 Zhang Cen wrote:
> A 6LoWPAN peer stores the protocol-owned L2CAP channel reference in
> peer->chan. chan_close_cb() removes the peer from the RCU list, but it
> also drops that reference immediately. The peer object can still be seen
> by in-flight RCU readers, and some paths keep using peer->chan after the
> lookup has finished.
> 
> That lets transmit and disconnect paths dereference, lock, or send
> through a channel whose last reference was released by the close path.
> 
Sounds like the race between close and lookup paths

	chan_close_cb()		setup_header()
	---			---
	peer_del()
	  list_del_rcu(&peer->list);
	  kfree_rcu(peer, rcu);
	l2cap_chan_put(c); // free chan

				peer_lookup_dst(dev, &ipv6_daddr, skb);
				  rcu_read_lock();
				  peer = find peer;
				  rcu_read_unlock();
				if (peer)
					addr = peer->chan->dst; //uaf

and on the lookup side, a) peer is unsafe outside rcu lock and
b) chan is used after put.

To fix the race, in addition to move l2cap_chan_put() to the rcu
callback as this patch does, refcount should be added to peer to
make the lookup path safe.

	chan_close_cb()		setup_header()
	---			---
	peer_del()
	  list_del_rcu(&peer->list);
	  call_rcu(&peer->rcu, peer_free_rcu);
	    if (peer->refcount-- == 0) {
	        //alternatively schedule workqueue if task context needed
	    	l2cap_chan_put(c);
		kfree(peer);
		module_put(THIS_MODULE);
	    }
				peer_lookup_dst(dev, &ipv6_daddr, skb);
				  rcu_read_lock();
				  peer = find peer;
				  if (peer)
				  	peer->refcount++;
				  rcu_read_unlock();
				if (peer) {
					addr = peer->chan->dst;
					if (peer->refcount-- == 0)
						schedule workqueue to free peer;
				}

> Defer the peer-owned l2cap_chan_put() until the peer RCU callback so a
> peer that remains observable through RCU still carries a live channel
> reference. Then take temporary channel references in the unicast,
> multicast, and explicit disconnect paths before they keep using the
> channel after the lookup window closes.
> 
> If the reader reaches step 3 after teardown reaches step 4, it can hit a
> freed l2cap_chan.
> 
> The buggy scenario involves two paths, with each column showing the order within that path:
> 
>   L2CAP peer teardown:                   Concurrent peer reader:
>   1. l2cap_conn_del() or another         1. A reader such as
>      close path takes a temporary           __peer_lookup_conn(),
>      hold on the channel                    setup_header(),
>                                             send_mcast_pkt(), or
>                                             bt_6lowpan_disconnect()
>                                             traverses the peer list and
>                                             reads peer->chan
>   2. l2cap_chan_del() drops the          2. The reader keeps using the
>      connection-owned channel               raw channel pointer after the
>      reference before 6LoWPAN               lookup or after only RCU
>      close handling finishes                protection remains
>   3. chan_close_cb() removes the         3. It dereferences channel
>      matching lowpan_peer from the          fields or calls send or close
>      peer list                              operations through that
>                                             pointer
>   4. The original chan_close_cb()
>      also dropped the peer-owned
>      l2cap_chan reference, and the
>      close caller later released
>      its temporary hold
> 
> A peer reader can still observe the lowpan_peer while chan_close_cb()
> has already consumed the peer-owned channel reference and the close
> caller is releasing the last temporary hold, leaving peer->chan stale
> before the peer's RCU grace period ends.
> 
> lowpan_peer objects stay RCU-visible after peer_del() removes them from
> the list.
> 
> Signed-off-by: Zhang Cen <rollkingzzc@gmail.com>
> 
> ---
> diff --git a/net/bluetooth/6lowpan.c b/net/bluetooth/6lowpan.c
> index 2f03b780b40d..ea3ee6929101 100644
> --- a/net/bluetooth/6lowpan.c
> +++ b/net/bluetooth/6lowpan.c
> @@ -95,13 +95,20 @@ static inline void peer_add(struct lowpan_btle_dev *dev,
>  	atomic_inc(&dev->peer_count);
>  }
>  
> +static void peer_free_rcu(struct rcu_head *rcu)
> +{
> +	struct lowpan_peer *peer = container_of(rcu, struct lowpan_peer, rcu);
> +
> +	l2cap_chan_put(peer->chan);
> +	kfree(peer);
> +	module_put(THIS_MODULE);
> +}
> +
>  static inline bool peer_del(struct lowpan_btle_dev *dev,
>  			    struct lowpan_peer *peer)
>  {
>  	list_del_rcu(&peer->list);
> -	kfree_rcu(peer, rcu);
> -
> -	module_put(THIS_MODULE);
> +	call_rcu(&peer->rcu, peer_free_rcu);
>  
>  	if (atomic_dec_and_test(&dev->peer_count)) {
>  		BT_DBG("last peer");
> @@ -137,9 +144,32 @@ __peer_lookup_conn(struct lowpan_btle_dev *dev, struct l2cap_conn *conn)
>  	return NULL;
>  }
>  
> -static inline struct lowpan_peer *peer_lookup_dst(struct lowpan_btle_dev *dev,
> -						  struct in6_addr *daddr,
> -						  struct sk_buff *skb)
> +static struct l2cap_chan *lookup_peer_chan(struct l2cap_conn *conn)
> +{
> +	struct lowpan_btle_dev *entry;
> +	struct lowpan_peer *peer;
> +	struct l2cap_chan *chan = NULL;
> +
> +	rcu_read_lock();
> +
> +	list_for_each_entry_rcu(entry, &bt_6lowpan_devices, list) {
> +		peer = __peer_lookup_conn(entry, conn);
> +		if (peer) {
> +			chan = peer->chan;
> +			l2cap_chan_hold(chan);
> +			break;
> +		}
> +	}
> +
> +	rcu_read_unlock();
> +
> +	return chan;
> +}
> +
> +static int peer_lookup_dst(struct lowpan_btle_dev *dev, struct in6_addr *daddr,
> +			   struct sk_buff *skb, u8 *lladdr,
> +			   bdaddr_t *peer_addr, u8 *peer_addr_type,
> +			   struct l2cap_chan **chan)
>  {
>  	struct rt6_info *rt = dst_rt6_info(skb_dst(skb));
>  	int count = atomic_read(&dev->peer_count);
> @@ -175,13 +205,20 @@ static inline struct lowpan_peer *peer_lookup_dst(struct lowpan_btle_dev *dev,
>  	rcu_read_lock();
>  
>  	list_for_each_entry_rcu(peer, &dev->peers, list) {
> +		struct l2cap_chan *pchan = peer->chan;
> +
>  		BT_DBG("dst addr %pMR dst type %u ip %pI6c",
> -		       &peer->chan->dst, peer->chan->dst_type,
> +		       &pchan->dst, pchan->dst_type,
>  		       &peer->peer_addr);
>  
>  		if (!ipv6_addr_cmp(&peer->peer_addr, nexthop)) {
> +			memcpy(lladdr, peer->lladdr, ETH_ALEN);
> +			*peer_addr = pchan->dst;
> +			*peer_addr_type = pchan->dst_type;
> +			l2cap_chan_hold(pchan);
> +			*chan = pchan;
>  			rcu_read_unlock();
> -			return peer;
> +			return 0;
>  		}
>  	}
>  
> @@ -190,9 +227,16 @@ static inline struct lowpan_peer *peer_lookup_dst(struct lowpan_btle_dev *dev,
>  	if (neigh) {
>  		list_for_each_entry_rcu(peer, &dev->peers, list) {
>  			if (!memcmp(neigh->ha, peer->lladdr, ETH_ALEN)) {
> +				struct l2cap_chan *pchan = peer->chan;
> +
> +				memcpy(lladdr, peer->lladdr, ETH_ALEN);
> +				*peer_addr = pchan->dst;
> +				*peer_addr_type = pchan->dst_type;
> +				l2cap_chan_hold(pchan);
> +				*chan = pchan;
>  				neigh_release(neigh);
>  				rcu_read_unlock();
> -				return peer;
> +				return 0;
>  			}
>  		}
>  		neigh_release(neigh);
> @@ -200,7 +244,7 @@ static inline struct lowpan_peer *peer_lookup_dst(struct lowpan_btle_dev *dev,
>  
>  	rcu_read_unlock();
>  
> -	return NULL;
> +	return -ENOENT;
>  }
>  
>  static struct lowpan_peer *lookup_peer(struct l2cap_conn *conn)
> @@ -379,7 +423,7 @@ static int setup_header(struct sk_buff *skb, struct net_device *netdev,
>  	struct in6_addr ipv6_daddr;
>  	struct ipv6hdr *hdr;
>  	struct lowpan_btle_dev *dev;
> -	struct lowpan_peer *peer;
> +	u8 peer_lladdr[ETH_ALEN];
>  	u8 *daddr;
>  	int err, status = 0;
>  
> @@ -388,9 +432,9 @@ static int setup_header(struct sk_buff *skb, struct net_device *netdev,
>  	dev = lowpan_btle_dev(netdev);
>  
>  	memcpy(&ipv6_daddr, &hdr->daddr, sizeof(ipv6_daddr));
> +	lowpan_cb(skb)->chan = NULL;
>  
>  	if (ipv6_addr_is_multicast(&ipv6_daddr)) {
> -		lowpan_cb(skb)->chan = NULL;
>  		daddr = NULL;
>  	} else {
>  		BT_DBG("dest IP %pI6c", &ipv6_daddr);
> @@ -400,16 +444,15 @@ static int setup_header(struct sk_buff *skb, struct net_device *netdev,
>  		 * or user set route) so get peer according to
>  		 * the destination address.
>  		 */
> -		peer = peer_lookup_dst(dev, &ipv6_daddr, skb);
> -		if (!peer) {
> +		err = peer_lookup_dst(dev, &ipv6_daddr, skb, peer_lladdr,
> +				      peer_addr, peer_addr_type,
> +				      &lowpan_cb(skb)->chan);
> +		if (err) {
>  			BT_DBG("no such peer");
> -			return -ENOENT;
> +			return err;
>  		}
>  
> -		daddr = peer->lladdr;
> -		*peer_addr = peer->chan->dst;
> -		*peer_addr_type = peer->chan->dst_type;
> -		lowpan_cb(skb)->chan = peer->chan;
> +		daddr = peer_lladdr;
>  
>  		status = 1;
>  	}
> @@ -417,8 +460,13 @@ static int setup_header(struct sk_buff *skb, struct net_device *netdev,
>  	lowpan_header_compress(skb, netdev, daddr, dev->netdev->dev_addr);
>  
>  	err = dev_hard_header(skb, netdev, ETH_P_IPV6, NULL, NULL, 0);
> -	if (err < 0)
> +	if (err < 0) {
> +		if (lowpan_cb(skb)->chan) {
> +			l2cap_chan_put(lowpan_cb(skb)->chan);
> +			lowpan_cb(skb)->chan = NULL;
> +		}
>  		return err;
> +	}
>  
>  	return status;
>  }
> @@ -483,15 +531,23 @@ static int send_mcast_pkt(struct sk_buff *skb, struct net_device *netdev)
>  		dev = lowpan_btle_dev(entry->netdev);
>  
>  		list_for_each_entry_rcu(pentry, &dev->peers, list) {
> +			struct l2cap_chan *chan = pentry->chan;
>  			int ret;
>  
>  			local_skb = skb_clone(skb, GFP_ATOMIC);
> +			if (!local_skb) {
> +				err = -ENOMEM;
> +				continue;
> +			}
> +
> +			l2cap_chan_hold(chan);
>  
>  			BT_DBG("xmit %s to %pMR type %u IP %pI6c chan %p",
>  			       netdev->name,
> -			       &pentry->chan->dst, pentry->chan->dst_type,
> -			       &pentry->peer_addr, pentry->chan);
> -			ret = send_pkt(pentry->chan, local_skb, netdev);
> +			       &chan->dst, chan->dst_type,
> +			       &pentry->peer_addr, chan);
> +			ret = send_pkt(chan, local_skb, netdev);
> +			l2cap_chan_put(chan);
>  			if (ret < 0)
>  				err = ret;
>  
> @@ -530,10 +586,14 @@ static netdev_tx_t bt_xmit(struct sk_buff *skb, struct net_device *netdev)
>  
>  	if (err) {
>  		if (lowpan_cb(skb)->chan) {
> +			struct l2cap_chan *chan = lowpan_cb(skb)->chan;
> +
>  			BT_DBG("xmit %s to %pMR type %u IP %pI6c chan %p",
>  			       netdev->name, &addr, addr_type,
> -			       &lowpan_cb(skb)->addr, lowpan_cb(skb)->chan);
> -			err = send_pkt(lowpan_cb(skb)->chan, skb, netdev);
> +			       &lowpan_cb(skb)->addr, chan);
> +			err = send_pkt(chan, skb, netdev);
> +			l2cap_chan_put(chan);
> +			lowpan_cb(skb)->chan = NULL;
>  		} else {
>  			err = -ENOENT;
>  		}
> @@ -802,8 +862,6 @@ static void chan_close_cb(struct l2cap_chan *chan)
>  			       last ? "last " : "1 ", peer);
>  			BT_DBG("chan %p orig refcnt %u", chan,
>  			       kref_read(&chan->kref));
> -
> -			l2cap_chan_put(chan);
>  			break;
>  		}
>  	}
> @@ -917,19 +975,20 @@ static int bt_6lowpan_connect(bdaddr_t *addr, u8 dst_type)
>  
>  static int bt_6lowpan_disconnect(struct l2cap_conn *conn, u8 dst_type)
>  {
> -	struct lowpan_peer *peer;
> +	struct l2cap_chan *chan;
>  
>  	BT_DBG("conn %p dst type %u", conn, dst_type);
>  
> -	peer = lookup_peer(conn);
> -	if (!peer)
> +	chan = lookup_peer_chan(conn);
> +	if (!chan)
>  		return -ENOENT;
>  
> -	BT_DBG("peer %p chan %p", peer, peer->chan);
> +	BT_DBG("chan %p", chan);
>  
> -	l2cap_chan_lock(peer->chan);
> -	l2cap_chan_close(peer->chan, ENOENT);
> -	l2cap_chan_unlock(peer->chan);
> +	l2cap_chan_lock(chan);
> +	l2cap_chan_close(chan, ENOENT);
> +	l2cap_chan_unlock(chan);
> +	l2cap_chan_put(chan);
>  
>  	return 0;
>  }
> 

  parent reply	other threads:[~2026-05-10  1:44 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-09 17:37 [PATCH] Bluetooth: 6lowpan: Defer peer channel release until RCU cleanup Zhang Cen
2026-05-09 18:29 ` bluez.test.bot
2026-05-10  1:43 ` Hillf Danton [this message]
2026-05-10  4:01   ` [PATCH] " c Z
2026-05-10  4:04 ` [PATCH v2] Bluetooth: 6lowpan: Fix peer and channel lifetime during teardown Zhang Cen
2026-05-10  4:38   ` [v2] " bluez.test.bot
2026-05-11 16:20   ` [PATCH v2] " Luiz Augusto von Dentz
2026-05-11 17:11     ` Cen Zhang
2026-05-11 17:18   ` [PATCH v3] " Zhang Cen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260510014359.472-1-hdanton@sina.com \
    --to=hdanton@sina.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    --cc=marcel@holtmann.org \
    --cc=rollkingzzc@gmail.com \
    --cc=zerocling0077@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.