[PATCH] net: bonding: avoid use-after-free with tx_hashtbl/rx_hashtbl

[PATCH] net: bonding: avoid use-after-free with tx_hashtbl/rx_hashtbl

[sujing]

In bonding mode 6 (Balance-alb),
there are some potential race conditions between the 'bond_close' process
and the tx/rx processes that use tx_hashtbl/rx_hashtbl,
which may lead to use-after-free.

For instance, when the bond6 device is in the 'bond_close' process
while some backlogged packets from upper level are transmitted
to 'bond_start_xmit', there is a spinlock contention between
'tlb_deinitialize' and 'tlb_choose_channel'.

If 'tlb_deinitialize' preempts the lock before 'tlb_choose_channel',
a NULL pointer kernel panic will be triggered.

Here's the timeline:

bond_close  ------------------  bond_start_xmit
bond_alb_deinitialize  -------  __bond_start_xmit
tlb_deinitialize  ------------  bond_alb_xmit
spin_lock_bh  ----------------  bond_xmit_alb_slave_get
tx_hashtbl = NULL  -----------  tlb_choose_channel
spin_unlock_bh  --------------  //wait for spin_lock_bh
------------------------------  spin_lock_bh
------------------------------  __tlb_choose_channel
causing kernel panic ========>  tx_hashtbl[hash_index].tx_slave
------------------------------  spin_unlock_bh

Signed-off-by: sujing <sujing@kylinos.cn>
---
 drivers/net/bonding/bond_alb.c  | 32 +++++++++------------------
 drivers/net/bonding/bond_main.c | 39 +++++++++++++++++++++++++++------
 include/net/bond_alb.h          |  5 ++++-
 3 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index b9dbad3a8af8..f6ff5ea835c4 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -71,7 +71,7 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
 
 /*********************** tlb specific functions ***************************/
 
-static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
+void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
 {
 	if (save_load) {
 		entry->load_history = 1 + entry->tx_bytes /
@@ -269,8 +269,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
 	spin_unlock_bh(&bond->mode_lock);
 }
 
-static int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
-			struct slave *slave)
+int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
+		 struct slave *slave)
 {
 	struct arp_pkt *arp, _arp;
 
@@ -756,7 +756,7 @@ static void rlb_init_table_entry_src(struct rlb_client_info *entry)
 	entry->src_next = RLB_NULL_INDEX;
 }
 
-static void rlb_init_table_entry(struct rlb_client_info *entry)
+void rlb_init_table_entry(struct rlb_client_info *entry)
 {
 	memset(entry, 0, sizeof(struct rlb_client_info));
 	rlb_init_table_entry_dst(entry);
@@ -874,9 +874,6 @@ static int rlb_initialize(struct bonding *bond)
 
 	spin_unlock_bh(&bond->mode_lock);
 
-	/* register to receive ARPs */
-	bond->recv_probe = rlb_arp_recv;
-
 	return 0;
 }
 
@@ -888,7 +885,6 @@ static void rlb_deinitialize(struct bonding *bond)
 
 	kfree(bond_info->rx_hashtbl);
 	bond_info->rx_hashtbl = NULL;
-	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
 
 	spin_unlock_bh(&bond->mode_lock);
 }
@@ -1303,7 +1299,7 @@ static bool alb_determine_nd(struct sk_buff *skb, struct bonding *bond)
 
 /************************ exported alb functions ************************/
 
-int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
+int bond_alb_initialize(struct bonding *bond)
 {
 	int res;
 
@@ -1311,15 +1307,10 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
 	if (res)
 		return res;
 
-	if (rlb_enabled) {
-		res = rlb_initialize(bond);
-		if (res) {
-			tlb_deinitialize(bond);
-			return res;
-		}
-		bond->alb_info.rlb_enabled = 1;
-	} else {
-		bond->alb_info.rlb_enabled = 0;
+	res = rlb_initialize(bond);
+	if (res) {
+		tlb_deinitialize(bond);
+		return res;
 	}
 
 	return 0;
@@ -1327,12 +1318,9 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
 
 void bond_alb_deinitialize(struct bonding *bond)
 {
-	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
-
 	tlb_deinitialize(bond);
 
-	if (bond_info->rlb_enabled)
-		rlb_deinitialize(bond);
+	rlb_deinitialize(bond);
 }
 
 static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 236e5219c811..8fcb5d3ac0a2 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4217,6 +4217,7 @@ static int bond_open(struct net_device *bond_dev)
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct list_head *iter;
 	struct slave *slave;
+	int i;
 
 	if (BOND_MODE(bond) == BOND_MODE_ROUNDROBIN && !bond->rr_tx_counter) {
 		bond->rr_tx_counter = alloc_percpu(u32);
@@ -4239,11 +4240,29 @@ static int bond_open(struct net_device *bond_dev)
 	}
 
 	if (bond_is_lb(bond)) {
-		/* bond_alb_initialize must be called before the timer
-		 * is started.
-		 */
-		if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
-			return -ENOMEM;
+		struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
+
+		spin_lock_bh(&bond->mode_lock);
+
+		for (i = 0; i < TLB_HASH_TABLE_SIZE; i++)
+			tlb_init_table_entry(&bond_info->tx_hashtbl[i], 0);
+
+		spin_unlock_bh(&bond->mode_lock);
+
+		if (BOND_MODE(bond) == BOND_MODE_ALB) {
+			bond->alb_info.rlb_enabled = 1;
+			spin_lock_bh(&bond->mode_lock);
+
+			bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
+			for (i = 0; i < RLB_HASH_TABLE_SIZE; i++)
+				rlb_init_table_entry(bond_info->rx_hashtbl + i);
+
+			spin_unlock_bh(&bond->mode_lock);
+			bond->recv_probe = rlb_arp_recv;
+		} else {
+			bond->alb_info.rlb_enabled = 0;
+		}
+
 		if (bond->params.tlb_dynamic_lb || BOND_MODE(bond) == BOND_MODE_ALB)
 			queue_delayed_work(bond->wq, &bond->alb_work, 0);
 	}
@@ -4279,8 +4298,6 @@ static int bond_close(struct net_device *bond_dev)
 
 	bond_work_cancel_all(bond);
 	bond->send_peer_notif = 0;
-	if (bond_is_lb(bond))
-		bond_alb_deinitialize(bond);
 	bond->recv_probe = NULL;
 
 	if (bond_uses_primary(bond)) {
@@ -5854,6 +5871,8 @@ static void bond_uninit(struct net_device *bond_dev)
 	struct list_head *iter;
 	struct slave *slave;
 
+	bond_alb_deinitialize(bond);
+
 	bond_netpoll_cleanup(bond_dev);
 
 	/* Release the bonded slaves */
@@ -6295,6 +6314,12 @@ static int bond_init(struct net_device *bond_dev)
 	    bond_dev->addr_assign_type == NET_ADDR_PERM)
 		eth_hw_addr_random(bond_dev);
 
+	/* bond_alb_initialize must be called before the timer
+	 * is started.
+	 */
+	if (bond_alb_initialize(bond))
+		return -ENOMEM;
+
 	return 0;
 }
 
diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
index 9dc082b2d543..9fd16e20ef82 100644
--- a/include/net/bond_alb.h
+++ b/include/net/bond_alb.h
@@ -150,7 +150,7 @@ struct alb_bond_info {
 						 */
 };
 
-int bond_alb_initialize(struct bonding *bond, int rlb_enabled);
+int bond_alb_initialize(struct bonding *bond);
 void bond_alb_deinitialize(struct bonding *bond);
 int bond_alb_init_slave(struct bonding *bond, struct slave *slave);
 void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave);
@@ -165,5 +165,8 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
 void bond_alb_monitor(struct work_struct *);
 int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr);
 void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id);
+int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond, struct slave *slave);
+void tlb_init_table_entry(struct tlb_client_info *entry, int save_load);
+void rlb_init_table_entry(struct rlb_client_info *entry);
 #endif /* _NET_BOND_ALB_H */
 
-- 
2.27.0

Re: [PATCH] net: bonding: avoid use-after-free with tx_hashtbl/rx_hashtbl

[Jay Vosburgh]

sujing <sujing@kylinos.cn> wrote:

>In bonding mode 6 (Balance-alb),
>there are some potential race conditions between the 'bond_close' process
>and the tx/rx processes that use tx_hashtbl/rx_hashtbl,
>which may lead to use-after-free.
>
>For instance, when the bond6 device is in the 'bond_close' process
>while some backlogged packets from upper level are transmitted
>to 'bond_start_xmit', there is a spinlock contention between
>'tlb_deinitialize' and 'tlb_choose_channel'.
>
>If 'tlb_deinitialize' preempts the lock before 'tlb_choose_channel',
>a NULL pointer kernel panic will be triggered.
>
>Here's the timeline:
>
>bond_close  ------------------  bond_start_xmit
>bond_alb_deinitialize  -------  __bond_start_xmit
>tlb_deinitialize  ------------  bond_alb_xmit
>spin_lock_bh  ----------------  bond_xmit_alb_slave_get
>tx_hashtbl = NULL  -----------  tlb_choose_channel
>spin_unlock_bh  --------------  //wait for spin_lock_bh
>------------------------------  spin_lock_bh
>------------------------------  __tlb_choose_channel
>causing kernel panic ========>  tx_hashtbl[hash_index].tx_slave
>------------------------------  spin_unlock_bh

	I'm still thinking on the race here, but have some questions
below about the implementation in the meantime.

>Signed-off-by: sujing <sujing@kylinos.cn>
>---
> drivers/net/bonding/bond_alb.c  | 32 +++++++++------------------
> drivers/net/bonding/bond_main.c | 39 +++++++++++++++++++++++++++------
> include/net/bond_alb.h          |  5 ++++-
> 3 files changed, 46 insertions(+), 30 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>index b9dbad3a8af8..f6ff5ea835c4 100644
>--- a/drivers/net/bonding/bond_alb.c
>+++ b/drivers/net/bonding/bond_alb.c
>@@ -71,7 +71,7 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
> 
> /*********************** tlb specific functions ***************************/
> 
>-static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
>+void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
> {
> 	if (save_load) {
> 		entry->load_history = 1 + entry->tx_bytes /
>@@ -269,8 +269,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
> 	spin_unlock_bh(&bond->mode_lock);
> }
> 
>-static int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
>-			struct slave *slave)
>+int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
>+		 struct slave *slave)
> {
> 	struct arp_pkt *arp, _arp;
> 
>@@ -756,7 +756,7 @@ static void rlb_init_table_entry_src(struct rlb_client_info *entry)
> 	entry->src_next = RLB_NULL_INDEX;
> }
> 
>-static void rlb_init_table_entry(struct rlb_client_info *entry)
>+void rlb_init_table_entry(struct rlb_client_info *entry)
> {
> 	memset(entry, 0, sizeof(struct rlb_client_info));
> 	rlb_init_table_entry_dst(entry);
>@@ -874,9 +874,6 @@ static int rlb_initialize(struct bonding *bond)
> 
> 	spin_unlock_bh(&bond->mode_lock);
> 
>-	/* register to receive ARPs */
>-	bond->recv_probe = rlb_arp_recv;
>-
> 	return 0;
> }
> 
>@@ -888,7 +885,6 @@ static void rlb_deinitialize(struct bonding *bond)
> 
> 	kfree(bond_info->rx_hashtbl);
> 	bond_info->rx_hashtbl = NULL;
>-	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;

	Why remove this line?

> 	spin_unlock_bh(&bond->mode_lock);
> }
>@@ -1303,7 +1299,7 @@ static bool alb_determine_nd(struct sk_buff *skb, struct bonding *bond)
> 
> /************************ exported alb functions ************************/
> 
>-int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>+int bond_alb_initialize(struct bonding *bond)
> {
> 	int res;
> 
>@@ -1311,15 +1307,10 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
> 	if (res)
> 		return res;
> 
>-	if (rlb_enabled) {
>-		res = rlb_initialize(bond);
>-		if (res) {
>-			tlb_deinitialize(bond);
>-			return res;
>-		}
>-		bond->alb_info.rlb_enabled = 1;
>-	} else {
>-		bond->alb_info.rlb_enabled = 0;
>+	res = rlb_initialize(bond);
>+	if (res) {
>+		tlb_deinitialize(bond);
>+		return res;
> 	}
> 
> 	return 0;
>@@ -1327,12 +1318,9 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
> 
> void bond_alb_deinitialize(struct bonding *bond)
> {
>-	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>-
> 	tlb_deinitialize(bond);
> 
>-	if (bond_info->rlb_enabled)
>-		rlb_deinitialize(bond);
>+	rlb_deinitialize(bond);

	Why is rlb_deinitialize() now unconditionally called here and in
bond_alb_initialize()?  if rlb_enabled is false, why set up / tear down
the RLB hash table that won't be used?

> }
> 
> static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 236e5219c811..8fcb5d3ac0a2 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4217,6 +4217,7 @@ static int bond_open(struct net_device *bond_dev)
> 	struct bonding *bond = netdev_priv(bond_dev);
> 	struct list_head *iter;
> 	struct slave *slave;
>+	int i;
> 
> 	if (BOND_MODE(bond) == BOND_MODE_ROUNDROBIN && !bond->rr_tx_counter) {
> 		bond->rr_tx_counter = alloc_percpu(u32);
>@@ -4239,11 +4240,29 @@ static int bond_open(struct net_device *bond_dev)
> 	}
> 
> 	if (bond_is_lb(bond)) {
>-		/* bond_alb_initialize must be called before the timer
>-		 * is started.
>-		 */
>-		if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
>-			return -ENOMEM;
>+		struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>+
>+		spin_lock_bh(&bond->mode_lock);
>+
>+		for (i = 0; i < TLB_HASH_TABLE_SIZE; i++)
>+			tlb_init_table_entry(&bond_info->tx_hashtbl[i], 0);
>+
>+		spin_unlock_bh(&bond->mode_lock);
>+
>+		if (BOND_MODE(bond) == BOND_MODE_ALB) {
>+			bond->alb_info.rlb_enabled = 1;
>+			spin_lock_bh(&bond->mode_lock);
>+
>+			bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
>+			for (i = 0; i < RLB_HASH_TABLE_SIZE; i++)
>+				rlb_init_table_entry(bond_info->rx_hashtbl + i);
>+
>+			spin_unlock_bh(&bond->mode_lock);
>+			bond->recv_probe = rlb_arp_recv;
>+		} else {
>+			bond->alb_info.rlb_enabled = 0;
>+		}
>+

	Why is all of the above done directly in bond_open() and not in
bond_alb.c somewhere?  That would reduce some churn (changing some
functions away from static).

	Also, I see that bond_alb_initialize() is now called from
bond_init() instead of bond_open(), and it only calls rlb_initialize().
However, this now duplicates most of the functionality of
rlb_initialize() and tlb_initialize() here.  Why?

	In general, the described race is TX vs. close processing, so
why is there so much change to the open processing?

	-J

> 		if (bond->params.tlb_dynamic_lb || BOND_MODE(bond) == BOND_MODE_ALB)
> 			queue_delayed_work(bond->wq, &bond->alb_work, 0);
> 	}
>@@ -4279,8 +4298,6 @@ static int bond_close(struct net_device *bond_dev)
> 
> 	bond_work_cancel_all(bond);
> 	bond->send_peer_notif = 0;
>-	if (bond_is_lb(bond))
>-		bond_alb_deinitialize(bond);
> 	bond->recv_probe = NULL;
> 
> 	if (bond_uses_primary(bond)) {
>@@ -5854,6 +5871,8 @@ static void bond_uninit(struct net_device *bond_dev)
> 	struct list_head *iter;
> 	struct slave *slave;
> 
>+	bond_alb_deinitialize(bond);
>+
> 	bond_netpoll_cleanup(bond_dev);
> 
> 	/* Release the bonded slaves */
>@@ -6295,6 +6314,12 @@ static int bond_init(struct net_device *bond_dev)
> 	    bond_dev->addr_assign_type == NET_ADDR_PERM)
> 		eth_hw_addr_random(bond_dev);
> 
>+	/* bond_alb_initialize must be called before the timer
>+	 * is started.
>+	 */
>+	if (bond_alb_initialize(bond))
>+		return -ENOMEM;
>+
> 	return 0;
> }
> 
>diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
>index 9dc082b2d543..9fd16e20ef82 100644
>--- a/include/net/bond_alb.h
>+++ b/include/net/bond_alb.h
>@@ -150,7 +150,7 @@ struct alb_bond_info {
> 						 */
> };
> 
>-int bond_alb_initialize(struct bonding *bond, int rlb_enabled);
>+int bond_alb_initialize(struct bonding *bond);
> void bond_alb_deinitialize(struct bonding *bond);
> int bond_alb_init_slave(struct bonding *bond, struct slave *slave);
> void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave);
>@@ -165,5 +165,8 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
> void bond_alb_monitor(struct work_struct *);
> int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr);
> void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id);
>+int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond, struct slave *slave);
>+void tlb_init_table_entry(struct tlb_client_info *entry, int save_load);
>+void rlb_init_table_entry(struct rlb_client_info *entry);
> #endif /* _NET_BOND_ALB_H */
> 
>-- 
>2.27.0
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH] net: bonding: avoid use-after-free with tx_hashtbl/rx_hashtbl

[Nikolay Aleksandrov]

On 28/03/2023 06:40, sujing wrote:
> In bonding mode 6 (Balance-alb),
> there are some potential race conditions between the 'bond_close' process
> and the tx/rx processes that use tx_hashtbl/rx_hashtbl,
> which may lead to use-after-free.

Potential and may? Have you seen it happen and have a trace?

> 
> For instance, when the bond6 device is in the 'bond_close' process
> while some backlogged packets from upper level are transmitted

How exactly would that happen? Queues get properly disabled before ndo_stop
is called.

> to 'bond_start_xmit', there is a spinlock contention between
> 'tlb_deinitialize' and 'tlb_choose_channel'.
> 
> If 'tlb_deinitialize' preempts the lock before 'tlb_choose_channel',
> a NULL pointer kernel panic will be triggered.
> 
> Here's the timeline:
> 
> bond_close  ------------------  bond_start_xmit
> bond_alb_deinitialize  -------  __bond_start_xmit
> tlb_deinitialize  ------------  bond_alb_xmit
> spin_lock_bh  ----------------  bond_xmit_alb_slave_get
> tx_hashtbl = NULL  -----------  tlb_choose_channel
> spin_unlock_bh  --------------  //wait for spin_lock_bh
> ------------------------------  spin_lock_bh
> ------------------------------  __tlb_choose_channel
> causing kernel panic ========>  tx_hashtbl[hash_index].tx_slave
> ------------------------------  spin_unlock_bh

I don't see how bond_close() can be called in parallel with bond_start_xmit.
Tx queues are disabled and there's a synchronize_rcu() before the device's
ndo_stop is called.

> 
> Signed-off-by: sujing <sujing@kylinos.cn>
> ---
>  drivers/net/bonding/bond_alb.c  | 32 +++++++++------------------
>  drivers/net/bonding/bond_main.c | 39 +++++++++++++++++++++++++++------
>  include/net/bond_alb.h          |  5 ++++-
>  3 files changed, 46 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index b9dbad3a8af8..f6ff5ea835c4 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -71,7 +71,7 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
>  
>  /*********************** tlb specific functions ***************************/
>  
> -static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
> +void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
>  {
>  	if (save_load) {
>  		entry->load_history = 1 + entry->tx_bytes /
> @@ -269,8 +269,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
>  	spin_unlock_bh(&bond->mode_lock);
>  }
>  
> -static int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
> -			struct slave *slave)
> +int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
> +		 struct slave *slave)
>  {
>  	struct arp_pkt *arp, _arp;
>  
> @@ -756,7 +756,7 @@ static void rlb_init_table_entry_src(struct rlb_client_info *entry)
>  	entry->src_next = RLB_NULL_INDEX;
>  }
>  
> -static void rlb_init_table_entry(struct rlb_client_info *entry)
> +void rlb_init_table_entry(struct rlb_client_info *entry)
>  {
>  	memset(entry, 0, sizeof(struct rlb_client_info));
>  	rlb_init_table_entry_dst(entry);
> @@ -874,9 +874,6 @@ static int rlb_initialize(struct bonding *bond)
>  
>  	spin_unlock_bh(&bond->mode_lock);
>  
> -	/* register to receive ARPs */
> -	bond->recv_probe = rlb_arp_recv;
> -
>  	return 0;
>  }
>  
> @@ -888,7 +885,6 @@ static void rlb_deinitialize(struct bonding *bond)
>  
>  	kfree(bond_info->rx_hashtbl);
>  	bond_info->rx_hashtbl = NULL;
> -	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
>  
>  	spin_unlock_bh(&bond->mode_lock);
>  }
> @@ -1303,7 +1299,7 @@ static bool alb_determine_nd(struct sk_buff *skb, struct bonding *bond)
>  
>  /************************ exported alb functions ************************/
>  
> -int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
> +int bond_alb_initialize(struct bonding *bond)
>  {
>  	int res;
>  
> @@ -1311,15 +1307,10 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>  	if (res)
>  		return res;
>  
> -	if (rlb_enabled) {
> -		res = rlb_initialize(bond);
> -		if (res) {
> -			tlb_deinitialize(bond);
> -			return res;
> -		}
> -		bond->alb_info.rlb_enabled = 1;
> -	} else {
> -		bond->alb_info.rlb_enabled = 0;
> +	res = rlb_initialize(bond);
> +	if (res) {
> +		tlb_deinitialize(bond);
> +		return res;
>  	}
>  
>  	return 0;
> @@ -1327,12 +1318,9 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>  
>  void bond_alb_deinitialize(struct bonding *bond)
>  {
> -	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> -
>  	tlb_deinitialize(bond);
>  
> -	if (bond_info->rlb_enabled)
> -		rlb_deinitialize(bond);
> +	rlb_deinitialize(bond);
>  }
>  
>  static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 236e5219c811..8fcb5d3ac0a2 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4217,6 +4217,7 @@ static int bond_open(struct net_device *bond_dev)
>  	struct bonding *bond = netdev_priv(bond_dev);
>  	struct list_head *iter;
>  	struct slave *slave;
> +	int i;
>  
>  	if (BOND_MODE(bond) == BOND_MODE_ROUNDROBIN && !bond->rr_tx_counter) {
>  		bond->rr_tx_counter = alloc_percpu(u32);
> @@ -4239,11 +4240,29 @@ static int bond_open(struct net_device *bond_dev)
>  	}
>  
>  	if (bond_is_lb(bond)) {
> -		/* bond_alb_initialize must be called before the timer
> -		 * is started.
> -		 */
> -		if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
> -			return -ENOMEM;
> +		struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
> +
> +		spin_lock_bh(&bond->mode_lock);
> +
> +		for (i = 0; i < TLB_HASH_TABLE_SIZE; i++)
> +			tlb_init_table_entry(&bond_info->tx_hashtbl[i], 0);
> +
> +		spin_unlock_bh(&bond->mode_lock);
> +
> +		if (BOND_MODE(bond) == BOND_MODE_ALB) {
> +			bond->alb_info.rlb_enabled = 1;
> +			spin_lock_bh(&bond->mode_lock);
> +
> +			bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
> +			for (i = 0; i < RLB_HASH_TABLE_SIZE; i++)
> +				rlb_init_table_entry(bond_info->rx_hashtbl + i);
> +
> +			spin_unlock_bh(&bond->mode_lock);
> +			bond->recv_probe = rlb_arp_recv;
> +		} else {
> +			bond->alb_info.rlb_enabled = 0;
> +		}
> +
>  		if (bond->params.tlb_dynamic_lb || BOND_MODE(bond) == BOND_MODE_ALB)
>  			queue_delayed_work(bond->wq, &bond->alb_work, 0);
>  	}
> @@ -4279,8 +4298,6 @@ static int bond_close(struct net_device *bond_dev)
>  
>  	bond_work_cancel_all(bond);
>  	bond->send_peer_notif = 0;
> -	if (bond_is_lb(bond))
> -		bond_alb_deinitialize(bond);
>  	bond->recv_probe = NULL;
>  
>  	if (bond_uses_primary(bond)) {
> @@ -5854,6 +5871,8 @@ static void bond_uninit(struct net_device *bond_dev)
>  	struct list_head *iter;
>  	struct slave *slave;
>  
> +	bond_alb_deinitialize(bond);
> +
>  	bond_netpoll_cleanup(bond_dev);
>  
>  	/* Release the bonded slaves */
> @@ -6295,6 +6314,12 @@ static int bond_init(struct net_device *bond_dev)
>  	    bond_dev->addr_assign_type == NET_ADDR_PERM)
>  		eth_hw_addr_random(bond_dev);
>  
> +	/* bond_alb_initialize must be called before the timer
> +	 * is started.
> +	 */
> +	if (bond_alb_initialize(bond))
> +		return -ENOMEM;
> +
>  	return 0;
>  }
>  
> diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
> index 9dc082b2d543..9fd16e20ef82 100644
> --- a/include/net/bond_alb.h
> +++ b/include/net/bond_alb.h
> @@ -150,7 +150,7 @@ struct alb_bond_info {
>  						 */
>  };
>  
> -int bond_alb_initialize(struct bonding *bond, int rlb_enabled);
> +int bond_alb_initialize(struct bonding *bond);
>  void bond_alb_deinitialize(struct bonding *bond);
>  int bond_alb_init_slave(struct bonding *bond, struct slave *slave);
>  void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave);
> @@ -165,5 +165,8 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
>  void bond_alb_monitor(struct work_struct *);
>  int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr);
>  void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id);
> +int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond, struct slave *slave);
> +void tlb_init_table_entry(struct tlb_client_info *entry, int save_load);
> +void rlb_init_table_entry(struct rlb_client_info *entry);
>  #endif /* _NET_BOND_ALB_H */
>  

Re: Re: [PATCH] net: bonding: avoid use-after-free with tx_hashtbl/rx_hashtbl

[sujing]

Thank you so much for your reply!

  I see there are mainly 5 questions about my patch.
  Before answering your questions, let me provide more detailed
information about this issue.

In bonding mode 6, I encounter 3 crashes on ft2000+/64(arm64) server
with kernel 4.19.90 (the issue still exists in the latest version).
Crashes only occur when closing process preempts the lock before TX/RX,
the call traces are as follow:

Call trace 1 : TX vs. close
[21406.614627] tlb_choose_channel+0x5c/0x188 [bonding]
[21406.620009] bond_alb_xmit+0x1fc/0x5b8 [bonding]
[21406.625043] bond_start_xmit+0xf8/0x4c0 [bonding]
[21406.630162] dev_hard_start_xmit+0xac/0x258
[21406.634759] __dev_queue_xmit+0x754/0x940
[21406.639182] dev_queue_xmit+0x24/0x30
[21406.643261] ip_finish_output2+0x23c/0x3e8
[21406.647771] ip_finish_output+0x1c8/0x2a8
[21406.652194] ip_output+0x9c/0x100
[21406.655924] ip_local_out+0x58/0x68
[21406.659829] __ip_queue_xmit+0x12c/0x368
[21406.664167] ip_queue_xmit+0x10/0x18
[21406.668158] __tcp_transmit_skb+0x4f8/0xab8
[21406.672754] tcp_write_xmit+0x23c/0xf78

Timeline 1:
tlb_deinitialize ------------------ bond_start_xmit
spin_lock_bh ---------------------- bond_alb_xmit
tx_hashtbl = NULL ----------------- tlb_choose_channel
spin_unlock_bh -------------------- //wait
----------------------------------- spin_lock_bh
----------------------------------- __tlb_choose_channel
----------------------------------- tx_hashtbl[hash_index].tx_slave
----------------------------------- spin_unlock_bh

  Call trace 2 : RX vs. close
[ 76.809874] Call trace:
[ 76.810317] rlb_arp_recv+0x218/0x2f0 [bonding]
[ 76.811070] bond_handle_frame+0x58/0x270 [bonding]
[ 76.811905] __netif_receive_skb_core+0x2d4/0xd90
[ 76.812683] __netif_receive_skb_one_core+0x38/0x68
[ 76.813477] __netif_receive_skb+0x28/0x80
[ 76.814156] netif_receive_skb_internal+0x3c/0xa8
[ 76.814929] napi_gro_receive+0xf8/0x170
[ 76.815594] receive_buf+0xec/0xa08 [virtio_net]
[ 76.816353] virtnet_poll+0x144/0x310 [virtio_net]
[ 76.817134] net_rx_action+0x158/0x3a0

Timeline 2:
rlb_deinitialize ------------------ bond_handle_frame
spin_lock_bh ---------------------- rlb_arp_recv
rx_hashtbl = NULL ----------------- rlb_update_entry_from_arp
spin_unlock_bh -------------------- //wait
----------------------------------- spin_lock_bh
----------------------------------- rx_hashtbl[hash_index].assigned
----------------------------------- spin_unlock_bh

  Call trace 3 : TX vs. close
[ 144.146818] Call trace:
[ 144.147256] rlb_choose_channel+0x84/0x2e8 [bonding]
[ 144.148067] bond_alb_xmit+0x2d0/0x5b8 [bonding]
[ 144.148820] bond_start_xmit+0x4e8/0x4f0 [bonding]
[ 144.149649] dev_hard_start_xmit+0xac/0x258
[ 144.150335] __dev_queue_xmit+0x754/0x940
[ 144.150994] dev_queue_xmit+0x24/0x30
[ 144.151616] arp_xmit+0x24/0x90
[ 144.152148] arp_send_dst.part.3+0xb4/0xf0
[ 144.152820] arp_solicit+0x1ac/0x260
[ 144.153416] neigh_probe+0x64/0x88
[ 144.153986] __neigh_event_send+0x124/0x340
[ 144.154671] neigh_resolve_output+0x124/0x200
[ 144.155383] ip_finish_output2+0x138/0x3e8
[ 144.156055] ip_finish_output+0x1c8/0x2a8
[ 144.156720] ip_output+0x9c/0x100
[ 144.157276] ip_local_out+0x58/0x68
[ 144.157860] ip_send_skb+0x2c/0x80

Timeline 3:
tlb_deinitialize ------------------ bond_start_xmit
spin_lock_bh ---------------------- bond_alb_xmit
tx_hashtbl = NULL ----------------- rlb_arp_xmit
spin_unlock_bh -------------------- rlb_choose_channel
----------------------------------- spin_lock_bh
----------------------------------- rx_hashtbl[hash_index].assigned
----------------------------------- spin_unlock_bh

To solve the crashes above, the initial idea is to check whether
tx_hashtbl/rx_hashtbl is NULL before operating it ,
and then add some exception handling process.

In bonding driver, there is only one tx_hashtbl operation (in
'__tlb_choose_channel()'), TLB is easy to solve.
But there are so many rx_hashtbl operations that also aren't
checked whether the pointer is NULL.
It may cause other use-after-free while closing.

I am worried that the initial idea can't solve crash issues under other
uncertain race conditions, and it maybe affecting the normal process.

Besides, I think it is unnecessary to free and reallocate
TLB / RLB hash table every-time the bond device is down/up.
Therefore it should be allocated and freed once in
'bond_init()/bond_uninit()'.

Since we can't control whether upper level sends packets or not,
I think the best idea is to make sure tx_hashtbl/rx_hashtbl pointer will
not be set to NULL during the closing process.

Therefore, move 'bond_alb_deinitialize()' from 'bond_close()' to
'bond_uninit()', move 'bond_alb_initialize()' from 'bond_open()'
to 'bond_init()'.

So it becomes as below:
                                     enter(insmod)
bond_init------------------bond_init------------
bond_open------------------bond_alb_initialize--
bond_alb_initialize---==>--tlb_initialize-------
tlb_initialize-------------rlb_initialize-------
rlb_initialize-------------bond_open------------
                                     exit(rmmod)
bond_close-----------------bond_close-----------
bond_alb_deinitialize------bond_uninit----------
tlb_deinitialize------==>--bond_alb_deinitialize
rlb_deinitialize-----------tlb_deinitialize-----
bond_uninit----------------rlb_deinitialize-----

The original initialization is:
if (bond_is_lb(bond))
      bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB));
         -> tlb_initialize(bond);
         -> if (rlb_enabled)
                   rlb_initialize(bond);
bond_is_lb()
      -> return BOND_MODE(bond) == BOND_MODE_TLB ||
                        BOND_MODE(bond) == BOND_MODE_ALB;
rlb_enabled == (BOND_MODE(bond) == BOND_MODE_ALB);

The original de-initialization is:
if (bond_is_lb(bond))
      bond_alb_deinitialize(bond);
         -> tlb_deinitialize(bond);
         -> if (bond_info->rlb_enabled)
                    rlb_deinitialize(bond);


However, 'BOND_MODE(bond)' is set only after 'bond_init()', meaning that
there's no way to judge the bond device's mode in 'bond_init()'.
I haven't come up with any better idea, so I decide to set up / tear down
the TLB / RLB hash table even when they won't be used.

So the initialization changes to:
bond_alb_initialize(bond);
            -> tlb_initialize(bond);
            -> rlb_initialize(bond);

As well as the de initialization:
bond_alb_deinitialize(bond);
            -> tlb_deinitialize(bond);
            -> rlb_deinitialize(bond);

  Since the main points is the movement of applying and releasing
hash-table memory, the rest in 'bond_close() bond_open()' are keep in
original places.

  In order to make sure each time the status is clean, 'bond_open()'
should clear the TLB / RLB hash table before used.

The answers to your questions in the previous email are below.


On 2023/3/28 12:38, Jay Vosburgh wrote:
> sujing <sujing@kylinos.cn> wrote:
>
>> In bonding mode 6 (Balance-alb),
>> there are some potential race conditions between the 'bond_close' process
>> and the tx/rx processes that use tx_hashtbl/rx_hashtbl,
>> which may lead to use-after-free.
>>
>> For instance, when the bond6 device is in the 'bond_close' process
>> while some backlogged packets from upper level are transmitted
>> to 'bond_start_xmit', there is a spinlock contention between
>> 'tlb_deinitialize' and 'tlb_choose_channel'.
>>
>> If 'tlb_deinitialize' preempts the lock before 'tlb_choose_channel',
>> a NULL pointer kernel panic will be triggered.
>>
>> Here's the timeline:
>>
>> bond_close  ------------------  bond_start_xmit
>> bond_alb_deinitialize  -------  __bond_start_xmit
>> tlb_deinitialize  ------------  bond_alb_xmit
>> spin_lock_bh  ----------------  bond_xmit_alb_slave_get
>> tx_hashtbl = NULL  -----------  tlb_choose_channel
>> spin_unlock_bh  --------------  //wait for spin_lock_bh
>> ------------------------------  spin_lock_bh
>> ------------------------------  __tlb_choose_channel
>> causing kernel panic ========>  tx_hashtbl[hash_index].tx_slave
>> ------------------------------  spin_unlock_bh
> 	I'm still thinking on the race here, but have some questions
> below about the implementation in the meantime.
>
>> Signed-off-by: sujing <sujing@kylinos.cn>
>> ---
>> drivers/net/bonding/bond_alb.c  | 32 +++++++++------------------
>> drivers/net/bonding/bond_main.c | 39 +++++++++++++++++++++++++++------
>> include/net/bond_alb.h          |  5 ++++-
>> 3 files changed, 46 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index b9dbad3a8af8..f6ff5ea835c4 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -71,7 +71,7 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
>>
>> /*********************** tlb specific functions ***************************/
>>
>> -static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
>> +void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
>> {
>> 	if (save_load) {
>> 		entry->load_history = 1 + entry->tx_bytes /
>> @@ -269,8 +269,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
>> 	spin_unlock_bh(&bond->mode_lock);
>> }
>>
>> -static int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
>> -			struct slave *slave)
>> +int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
>> +		 struct slave *slave)
>> {
>> 	struct arp_pkt *arp, _arp;
>>
>> @@ -756,7 +756,7 @@ static void rlb_init_table_entry_src(struct rlb_client_info *entry)
>> 	entry->src_next = RLB_NULL_INDEX;
>> }
>>
>> -static void rlb_init_table_entry(struct rlb_client_info *entry)
>> +void rlb_init_table_entry(struct rlb_client_info *entry)
>> {
>> 	memset(entry, 0, sizeof(struct rlb_client_info));
>> 	rlb_init_table_entry_dst(entry);
>> @@ -874,9 +874,6 @@ static int rlb_initialize(struct bonding *bond)
>>
>> 	spin_unlock_bh(&bond->mode_lock);
>>
>> -	/* register to receive ARPs */
>> -	bond->recv_probe = rlb_arp_recv;
>> -
>> 	return 0;
>> }
>>
>> @@ -888,7 +885,6 @@ static void rlb_deinitialize(struct bonding *bond)
>>
>> 	kfree(bond_info->rx_hashtbl);
>> 	bond_info->rx_hashtbl = NULL;
>> -	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
> 	Why remove this line?

The idea is moving 'rlb_deinitialize()' from 'bond_close()' process to
'bond_uninit()' process, so there's no need to reset rx_hashtbl_used_head's
value while exiting the driver.

>> 	spin_unlock_bh(&bond->mode_lock);
>> }
>> @@ -1303,7 +1299,7 @@ static bool alb_determine_nd(struct sk_buff *skb, struct bonding *bond)
>>
>> /************************ exported alb functions ************************/
>>
>> -int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>> +int bond_alb_initialize(struct bonding *bond)
>> {
>> 	int res;
>>
>> @@ -1311,15 +1307,10 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>> 	if (res)
>> 		return res;
>>
>> -	if (rlb_enabled) {
>> -		res = rlb_initialize(bond);
>> -		if (res) {
>> -			tlb_deinitialize(bond);
>> -			return res;
>> -		}
>> -		bond->alb_info.rlb_enabled = 1;
>> -	} else {
>> -		bond->alb_info.rlb_enabled = 0;
>> +	res = rlb_initialize(bond);
>> +	if (res) {
>> +		tlb_deinitialize(bond);
>> +		return res;
>> 	}
>>
>> 	return 0;
>> @@ -1327,12 +1318,9 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>>
>> void bond_alb_deinitialize(struct bonding *bond)
>> {
>> -	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> -
>> 	tlb_deinitialize(bond);
>>
>> -	if (bond_info->rlb_enabled)
>> -		rlb_deinitialize(bond);
>> +	rlb_deinitialize(bond);
> 	Why is rlb_deinitialize() now unconditionally called here and in
> bond_alb_initialize()?  if rlb_enabled is false, why set up / tear down
> the RLB hash table that won't be used?

There may be some misunderstanding. 'rlb_deinitialize()' and
'tlb_deinitialize()'  are now called in 'bond_alb_deinitialize()'.

As for the next question, I explain it in the beginning, there's no way to
judge the bond device's mode in 'bond_init()'.
I haven't come up with a better way.

>> }
>>
>> static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 236e5219c811..8fcb5d3ac0a2 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4217,6 +4217,7 @@ static int bond_open(struct net_device *bond_dev)
>> 	struct bonding *bond = netdev_priv(bond_dev);
>> 	struct list_head *iter;
>> 	struct slave *slave;
>> +	int i;
>>
>> 	if (BOND_MODE(bond) == BOND_MODE_ROUNDROBIN && !bond->rr_tx_counter) {
>> 		bond->rr_tx_counter = alloc_percpu(u32);
>> @@ -4239,11 +4240,29 @@ static int bond_open(struct net_device *bond_dev)
>> 	}
>>
>> 	if (bond_is_lb(bond)) {
>> -		/* bond_alb_initialize must be called before the timer
>> -		 * is started.
>> -		 */
>> -		if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
>> -			return -ENOMEM;
>> +		struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> +
>> +		spin_lock_bh(&bond->mode_lock);
>> +
>> +		for (i = 0; i < TLB_HASH_TABLE_SIZE; i++)
>> +			tlb_init_table_entry(&bond_info->tx_hashtbl[i], 0);
>> +
>> +		spin_unlock_bh(&bond->mode_lock);
>> +
>> +		if (BOND_MODE(bond) == BOND_MODE_ALB) {
>> +			bond->alb_info.rlb_enabled = 1;
>> +			spin_lock_bh(&bond->mode_lock);
>> +
>> +			bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
>> +			for (i = 0; i < RLB_HASH_TABLE_SIZE; i++)
>> +				rlb_init_table_entry(bond_info->rx_hashtbl + i);
>> +
>> +			spin_unlock_bh(&bond->mode_lock);
>> +			bond->recv_probe = rlb_arp_recv;
>> +		} else {
>> +			bond->alb_info.rlb_enabled = 0;
>> +		}
>> +
> 	Why is all of the above done directly in bond_open() and not in
> bond_alb.c somewhere?  That would reduce some churn (changing some
> functions away from static).
>
> 	Also, I see that bond_alb_initialize() is now called from
> bond_init() instead of bond_open(), and it only calls rlb_initialize().
> However, this now duplicates most of the functionality of
> rlb_initialize() and tlb_initialize() here.  Why?
>
> 	In general, the described race is TX vs. close processing, so
> why is there so much change to the open processing?
>
> 	-J

For the record, 'bond_alb_initialize()' now calls both 'rlb_initialize()'
and 'tlb_initialize()'.

You're right. There are two points that need to be improved.

point 1: create 2 new functions 'rlb_hash_initialize()' and
'tlb_hash_initialize()' in bond_alb.c, make them do all of the above.
The plan is as follow:
***************************************
bond_open() in bond_main.c:
if (bond_is_lb(bond)) {
      tlb_hash_initialize(bond);
      if (BOND_MODE(bond) == BOND_MODE_ALB) {
           bond->alb_info.rlb_enabled = 1;
           rlb_hash_initialize(bond);
           bond->recv_probe = rlb_arp_recv;
      } else
           bond->alb_info.rlb_enabled = 0;
}

tlb_hash_initialize() in bond_alb.c:
spin_lock_bh();
for (i)
      tlb_init_table_entry();
spin_unlock_bh();

rlb_hash_initialize() in bond_alb.c:
spin_lock_bh();
rx_hashtbl_used_head = RLB_NULL_INDEX;
for (i)
      rlb_init_table_entry();
spin_unlock_bh();

***************************************

point 2: delete the 'rlb_init_table_entry()' in 'rlb_initialize()' and
'tlb_init_table_entry()' in 'tlb_initialize()', there shouldn't double
the operations.

The key-point is avoiding TLB/RLB hash table is set to NULL while closing.
As for the last question, I think this issue due to timeline between
close processing and TLB / RLB operating.

If all these resetting operations are in 'bond_close()' instead of
'bond_open()', there still exist the same race conditions.

For example:
bond_close------------bond_start_xmit
spin_lock_bh----------bond_alb_xmit
tlb_init_table_entry--tlb_choose_channel
spin_unlock_bh--------//wait for lock
----------------------spin_lock_bh
----------------------tx_hashtbl[hash_index].tx_slave
----------------------spin_unlock_bh

If 'bond_close()' preempts the lock before TX, it won't be able to choose
the correct tx_slave in 'tlb_choose_channel()'.

However, if all these are in 'bond_open()', when the same race conditions
happen, TX can still work normally.


  Looking forward to your opinion on my reply!

-sujing

>> 		if (bond->params.tlb_dynamic_lb || BOND_MODE(bond) == BOND_MODE_ALB)
>> 			queue_delayed_work(bond->wq, &bond->alb_work, 0);
>> 	}
>> @@ -4279,8 +4298,6 @@ static int bond_close(struct net_device *bond_dev)
>>
>> 	bond_work_cancel_all(bond);
>> 	bond->send_peer_notif = 0;
>> -	if (bond_is_lb(bond))
>> -		bond_alb_deinitialize(bond);
>> 	bond->recv_probe = NULL;
>>
>> 	if (bond_uses_primary(bond)) {
>> @@ -5854,6 +5871,8 @@ static void bond_uninit(struct net_device *bond_dev)
>> 	struct list_head *iter;
>> 	struct slave *slave;
>>
>> +	bond_alb_deinitialize(bond);
>> +
>> 	bond_netpoll_cleanup(bond_dev);
>>
>> 	/* Release the bonded slaves */
>> @@ -6295,6 +6314,12 @@ static int bond_init(struct net_device *bond_dev)
>> 	    bond_dev->addr_assign_type == NET_ADDR_PERM)
>> 		eth_hw_addr_random(bond_dev);
>>
>> +	/* bond_alb_initialize must be called before the timer
>> +	 * is started.
>> +	 */
>> +	if (bond_alb_initialize(bond))
>> +		return -ENOMEM;
>> +
>> 	return 0;
>> }
>>
>> diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
>> index 9dc082b2d543..9fd16e20ef82 100644
>> --- a/include/net/bond_alb.h
>> +++ b/include/net/bond_alb.h
>> @@ -150,7 +150,7 @@ struct alb_bond_info {
>> 						 */
>> };
>>
>> -int bond_alb_initialize(struct bonding *bond, int rlb_enabled);
>> +int bond_alb_initialize(struct bonding *bond);
>> void bond_alb_deinitialize(struct bonding *bond);
>> int bond_alb_init_slave(struct bonding *bond, struct slave *slave);
>> void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave);
>> @@ -165,5 +165,8 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
>> void bond_alb_monitor(struct work_struct *);
>> int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr);
>> void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id);
>> +int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond, struct slave *slave);
>> +void tlb_init_table_entry(struct tlb_client_info *entry, int save_load);
>> +void rlb_init_table_entry(struct rlb_client_info *entry);
>> #endif /* _NET_BOND_ALB_H */
>>
>> -- 
>> 2.27.0
>>
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com
---
                     -sujing    <sujing@kyinos.cn>

Re: [PATCH] net: bonding: avoid use-after-free with tx_hashtbl/rx_hashtbl

[sujing]

Thank you so much for your reply!

  I see there are mainly 5 questions about my patch.
  Before answering your questions, let me provide more detailed
information about this issue.

In bonding mode 6, I encounter 3 crashes on ft2000+/64(arm64) server
with kernel 4.19.90 (the issue still exists in the latest version).
Crashes only occur when closing process preempts the lock before TX/RX,
the call traces are as follow:

Call trace 1 : TX vs. close
[21406.614627] tlb_choose_channel+0x5c/0x188 [bonding]
[21406.620009] bond_alb_xmit+0x1fc/0x5b8 [bonding]
[21406.625043] bond_start_xmit+0xf8/0x4c0 [bonding]
[21406.630162] dev_hard_start_xmit+0xac/0x258
[21406.634759] __dev_queue_xmit+0x754/0x940
[21406.639182] dev_queue_xmit+0x24/0x30
[21406.643261] ip_finish_output2+0x23c/0x3e8
[21406.647771] ip_finish_output+0x1c8/0x2a8
[21406.652194] ip_output+0x9c/0x100
[21406.655924] ip_local_out+0x58/0x68
[21406.659829] __ip_queue_xmit+0x12c/0x368
[21406.664167] ip_queue_xmit+0x10/0x18
[21406.668158] __tcp_transmit_skb+0x4f8/0xab8
[21406.672754] tcp_write_xmit+0x23c/0xf78

Timeline 1:
tlb_deinitialize ------------------ bond_start_xmit
spin_lock_bh ---------------------- bond_alb_xmit
tx_hashtbl = NULL ----------------- tlb_choose_channel
spin_unlock_bh -------------------- //wait
----------------------------------- spin_lock_bh
----------------------------------- __tlb_choose_channel
----------------------------------- tx_hashtbl[hash_index].tx_slave
----------------------------------- spin_unlock_bh

  Call trace 2 : RX vs. close
[ 76.809874] Call trace:
[ 76.810317] rlb_arp_recv+0x218/0x2f0 [bonding]
[ 76.811070] bond_handle_frame+0x58/0x270 [bonding]
[ 76.811905] __netif_receive_skb_core+0x2d4/0xd90
[ 76.812683] __netif_receive_skb_one_core+0x38/0x68
[ 76.813477] __netif_receive_skb+0x28/0x80
[ 76.814156] netif_receive_skb_internal+0x3c/0xa8
[ 76.814929] napi_gro_receive+0xf8/0x170
[ 76.815594] receive_buf+0xec/0xa08 [virtio_net]
[ 76.816353] virtnet_poll+0x144/0x310 [virtio_net]
[ 76.817134] net_rx_action+0x158/0x3a0

Timeline 2:
rlb_deinitialize ------------------ bond_handle_frame
spin_lock_bh ---------------------- rlb_arp_recv
rx_hashtbl = NULL ----------------- rlb_update_entry_from_arp
spin_unlock_bh -------------------- //wait
----------------------------------- spin_lock_bh
----------------------------------- rx_hashtbl[hash_index].assigned
----------------------------------- spin_unlock_bh

  Call trace 3 : TX vs. close
[ 144.146818] Call trace:
[ 144.147256] rlb_choose_channel+0x84/0x2e8 [bonding]
[ 144.148067] bond_alb_xmit+0x2d0/0x5b8 [bonding]
[ 144.148820] bond_start_xmit+0x4e8/0x4f0 [bonding]
[ 144.149649] dev_hard_start_xmit+0xac/0x258
[ 144.150335] __dev_queue_xmit+0x754/0x940
[ 144.150994] dev_queue_xmit+0x24/0x30
[ 144.151616] arp_xmit+0x24/0x90
[ 144.152148] arp_send_dst.part.3+0xb4/0xf0
[ 144.152820] arp_solicit+0x1ac/0x260
[ 144.153416] neigh_probe+0x64/0x88
[ 144.153986] __neigh_event_send+0x124/0x340
[ 144.154671] neigh_resolve_output+0x124/0x200
[ 144.155383] ip_finish_output2+0x138/0x3e8
[ 144.156055] ip_finish_output+0x1c8/0x2a8
[ 144.156720] ip_output+0x9c/0x100
[ 144.157276] ip_local_out+0x58/0x68
[ 144.157860] ip_send_skb+0x2c/0x80

Timeline 3:
tlb_deinitialize ------------------ bond_start_xmit
spin_lock_bh ---------------------- bond_alb_xmit
tx_hashtbl = NULL ----------------- rlb_arp_xmit
spin_unlock_bh -------------------- rlb_choose_channel
----------------------------------- spin_lock_bh
----------------------------------- rx_hashtbl[hash_index].assigned
----------------------------------- spin_unlock_bh

To solve the crashes above, the initial idea is to check whether
tx_hashtbl/rx_hashtbl is NULL before operating it ,
and then add some exception handling process.

In bonding driver, there is only one tx_hashtbl operation (in
'__tlb_choose_channel()'), TLB is easy to solve.
But there are so many rx_hashtbl operations that also aren't
checked whether the pointer is NULL.
It may cause other use-after-free while closing.

I am worried that the initial idea can't solve crash issues under other
uncertain race conditions, and it maybe affecting the normal process.

Besides, I think it is unnecessary to free and reallocate
TLB / RLB hash table every-time the bond device is down/up.
Therefore it should be allocated and freed once in
'bond_init()/bond_uninit()'.

Since we can't control whether upper level sends packets or not,
I think the best idea is to make sure tx_hashtbl/rx_hashtbl pointer will
not be set to NULL during the closing process.

Therefore, move 'bond_alb_deinitialize()' from 'bond_close()' to
'bond_uninit()', move 'bond_alb_initialize()' from 'bond_open()'
to 'bond_init()'.

So it becomes as below:
                                     enter(insmod)
bond_init------------------bond_init------------
bond_open------------------bond_alb_initialize--
bond_alb_initialize---==>--tlb_initialize-------
tlb_initialize-------------rlb_initialize-------
rlb_initialize-------------bond_open------------
                                     exit(rmmod)
bond_close-----------------bond_close-----------
bond_alb_deinitialize------bond_uninit----------
tlb_deinitialize------==>--bond_alb_deinitialize
rlb_deinitialize-----------tlb_deinitialize-----
bond_uninit----------------rlb_deinitialize-----

The original initialization is:
if (bond_is_lb(bond))
      bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB));
         -> tlb_initialize(bond);
         -> if (rlb_enabled)
                   rlb_initialize(bond);
bond_is_lb()
      -> return BOND_MODE(bond) == BOND_MODE_TLB ||
                        BOND_MODE(bond) == BOND_MODE_ALB;
rlb_enabled == (BOND_MODE(bond) == BOND_MODE_ALB);

The original de-initialization is:
if (bond_is_lb(bond))
      bond_alb_deinitialize(bond);
         -> tlb_deinitialize(bond);
         -> if (bond_info->rlb_enabled)
                    rlb_deinitialize(bond);

However, 'BOND_MODE(bond)' is set only after 'bond_init()', meaning that
there's no way to judge the bond device's mode in 'bond_init()'.
I haven't come up with any better idea, so I decide to set up / tear down
the TLB / RLB hash table even when they won't be used.

So the initialization changes to:
bond_alb_initialize(bond);
            -> tlb_initialize(bond);
            -> rlb_initialize(bond);

As well as the de initialization:
bond_alb_deinitialize(bond);
            -> tlb_deinitialize(bond);
            -> rlb_deinitialize(bond);

  Since the main points is the movement of applying and releasing
hash-table memory, the rest in 'bond_close() bond_open()' are keep in
original places.

  In order to make sure each time the status is clean, 'bond_open()'
should clear the TLB / RLB hash table before used.

The answers to your questions in the previous email are below.

On 2023/3/28 12:38, Jay Vosburgh wrote:
> sujing <sujing@kylinos.cn> wrote:
>
>> In bonding mode 6 (Balance-alb),
>> there are some potential race conditions between the 'bond_close' process
>> and the tx/rx processes that use tx_hashtbl/rx_hashtbl,
>> which may lead to use-after-free.
>>
>> For instance, when the bond6 device is in the 'bond_close' process
>> while some backlogged packets from upper level are transmitted
>> to 'bond_start_xmit', there is a spinlock contention between
>> 'tlb_deinitialize' and 'tlb_choose_channel'.
>>
>> If 'tlb_deinitialize' preempts the lock before 'tlb_choose_channel',
>> a NULL pointer kernel panic will be triggered.
>>
>> Here's the timeline:
>>
>> bond_close  ------------------  bond_start_xmit
>> bond_alb_deinitialize  -------  __bond_start_xmit
>> tlb_deinitialize  ------------  bond_alb_xmit
>> spin_lock_bh  ----------------  bond_xmit_alb_slave_get
>> tx_hashtbl = NULL  -----------  tlb_choose_channel
>> spin_unlock_bh  --------------  //wait for spin_lock_bh
>> ------------------------------  spin_lock_bh
>> ------------------------------  __tlb_choose_channel
>> causing kernel panic ========>  tx_hashtbl[hash_index].tx_slave
>> ------------------------------  spin_unlock_bh
> 	I'm still thinking on the race here, but have some questions
> below about the implementation in the meantime.
>
>> Signed-off-by: sujing <sujing@kylinos.cn>
>> ---
>> drivers/net/bonding/bond_alb.c  | 32 +++++++++------------------
>> drivers/net/bonding/bond_main.c | 39 +++++++++++++++++++++++++++------
>> include/net/bond_alb.h          |  5 ++++-
>> 3 files changed, 46 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
>> index b9dbad3a8af8..f6ff5ea835c4 100644
>> --- a/drivers/net/bonding/bond_alb.c
>> +++ b/drivers/net/bonding/bond_alb.c
>> @@ -71,7 +71,7 @@ static inline u8 _simple_hash(const u8 *hash_start, int hash_size)
>>
>> /*********************** tlb specific functions ***************************/
>>
>> -static inline void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
>> +void tlb_init_table_entry(struct tlb_client_info *entry, int save_load)
>> {
>> 	if (save_load) {
>> 		entry->load_history = 1 + entry->tx_bytes /
>> @@ -269,8 +269,8 @@ static void rlb_update_entry_from_arp(struct bonding *bond, struct arp_pkt *arp)
>> 	spin_unlock_bh(&bond->mode_lock);
>> }
>>
>> -static int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
>> -			struct slave *slave)
>> +int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond,
>> +		 struct slave *slave)
>> {
>> 	struct arp_pkt *arp, _arp;
>>
>> @@ -756,7 +756,7 @@ static void rlb_init_table_entry_src(struct rlb_client_info *entry)
>> 	entry->src_next = RLB_NULL_INDEX;
>> }
>>
>> -static void rlb_init_table_entry(struct rlb_client_info *entry)
>> +void rlb_init_table_entry(struct rlb_client_info *entry)
>> {
>> 	memset(entry, 0, sizeof(struct rlb_client_info));
>> 	rlb_init_table_entry_dst(entry);
>> @@ -874,9 +874,6 @@ static int rlb_initialize(struct bonding *bond)
>>
>> 	spin_unlock_bh(&bond->mode_lock);
>>
>> -	/* register to receive ARPs */
>> -	bond->recv_probe = rlb_arp_recv;
>> -
>> 	return 0;
>> }
>>
>> @@ -888,7 +885,6 @@ static void rlb_deinitialize(struct bonding *bond)
>>
>> 	kfree(bond_info->rx_hashtbl);
>> 	bond_info->rx_hashtbl = NULL;
>> -	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
> 	Why remove this line?

The idea is moving 'rlb_deinitialize()' from 'bond_close()' process to
'bond_uninit()' process, so there's no need to reset rx_hashtbl_used_head's
value while exiting the driver.

>> 	spin_unlock_bh(&bond->mode_lock);
>> }
>> @@ -1303,7 +1299,7 @@ static bool alb_determine_nd(struct sk_buff *skb, struct bonding *bond)
>>
>> /************************ exported alb functions ************************/
>>
>> -int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>> +int bond_alb_initialize(struct bonding *bond)
>> {
>> 	int res;
>>
>> @@ -1311,15 +1307,10 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>> 	if (res)
>> 		return res;
>>
>> -	if (rlb_enabled) {
>> -		res = rlb_initialize(bond);
>> -		if (res) {
>> -			tlb_deinitialize(bond);
>> -			return res;
>> -		}
>> -		bond->alb_info.rlb_enabled = 1;
>> -	} else {
>> -		bond->alb_info.rlb_enabled = 0;
>> +	res = rlb_initialize(bond);
>> +	if (res) {
>> +		tlb_deinitialize(bond);
>> +		return res;
>> 	}
>>
>> 	return 0;
>> @@ -1327,12 +1318,9 @@ int bond_alb_initialize(struct bonding *bond, int rlb_enabled)
>>
>> void bond_alb_deinitialize(struct bonding *bond)
>> {
>> -	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> -
>> 	tlb_deinitialize(bond);
>>
>> -	if (bond_info->rlb_enabled)
>> -		rlb_deinitialize(bond);
>> +	rlb_deinitialize(bond);
> 	Why is rlb_deinitialize() now unconditionally called here and in
> bond_alb_initialize()?  if rlb_enabled is false, why set up / tear down
> the RLB hash table that won't be used?
>

There may be some misunderstanding. 'rlb_deinitialize()' and
'tlb_deinitialize()'  are now called in 'bond_alb_deinitialize()'.

As for the next question, I explain it in the beginning, there's no way to
judge the bond device's mode in 'bond_init()'.
I haven't come up with a better way.

>> }
>>
>> static netdev_tx_t bond_do_alb_xmit(struct sk_buff *skb, struct bonding *bond,
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 236e5219c811..8fcb5d3ac0a2 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4217,6 +4217,7 @@ static int bond_open(struct net_device *bond_dev)
>> 	struct bonding *bond = netdev_priv(bond_dev);
>> 	struct list_head *iter;
>> 	struct slave *slave;
>> +	int i;
>>
>> 	if (BOND_MODE(bond) == BOND_MODE_ROUNDROBIN && !bond->rr_tx_counter) {
>> 		bond->rr_tx_counter = alloc_percpu(u32);
>> @@ -4239,11 +4240,29 @@ static int bond_open(struct net_device *bond_dev)
>> 	}
>>
>> 	if (bond_is_lb(bond)) {
>> -		/* bond_alb_initialize must be called before the timer
>> -		 * is started.
>> -		 */
>> -		if (bond_alb_initialize(bond, (BOND_MODE(bond) == BOND_MODE_ALB)))
>> -			return -ENOMEM;
>> +		struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> +
>> +		spin_lock_bh(&bond->mode_lock);
>> +
>> +		for (i = 0; i < TLB_HASH_TABLE_SIZE; i++)
>> +			tlb_init_table_entry(&bond_info->tx_hashtbl[i], 0);
>> +
>> +		spin_unlock_bh(&bond->mode_lock);
>> +
>> +		if (BOND_MODE(bond) == BOND_MODE_ALB) {
>> +			bond->alb_info.rlb_enabled = 1;
>> +			spin_lock_bh(&bond->mode_lock);
>> +
>> +			bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
>> +			for (i = 0; i < RLB_HASH_TABLE_SIZE; i++)
>> +				rlb_init_table_entry(bond_info->rx_hashtbl + i);
>> +
>> +			spin_unlock_bh(&bond->mode_lock);
>> +			bond->recv_probe = rlb_arp_recv;
>> +		} else {
>> +			bond->alb_info.rlb_enabled = 0;
>> +		}
>> +
> 	Why is all of the above done directly in bond_open() and not in
> bond_alb.c somewhere?  That would reduce some churn (changing some
> functions away from static).
>
> 	Also, I see that bond_alb_initialize() is now called from
> bond_init() instead of bond_open(), and it only calls rlb_initialize().
> However, this now duplicates most of the functionality of
> rlb_initialize() and tlb_initialize() here.  Why?
>
> 	In general, the described race is TX vs. close processing, so
> why is there so much change to the open processing?
>
> 	-J

For the record, 'bond_alb_initialize()' now calls both 'rlb_initialize()'
and 'tlb_initialize()'.

You're right. There are two points that need to be improved.

point 1: create 2 new functions 'rlb_hash_initialize()' and
'tlb_hash_initialize()' in bond_alb.c, make them do all of the above.
The plan is as follow:
***************************************
bond_open() in bond_main.c:
if (bond_is_lb(bond)) {
      tlb_hash_initialize(bond);
      if (BOND_MODE(bond) == BOND_MODE_ALB) {
           bond->alb_info.rlb_enabled = 1;
           rlb_hash_initialize(bond);
           bond->recv_probe = rlb_arp_recv;
      } else
           bond->alb_info.rlb_enabled = 0;
}

tlb_hash_initialize() in bond_alb.c:
spin_lock_bh();
for (i)
      tlb_init_table_entry();
spin_unlock_bh();

rlb_hash_initialize() in bond_alb.c:
spin_lock_bh();
rx_hashtbl_used_head = RLB_NULL_INDEX;
for (i)
      rlb_init_table_entry();
spin_unlock_bh();

***************************************

point 2: delete the 'rlb_init_table_entry()' in 'rlb_initialize()' and
'tlb_init_table_entry()' in 'tlb_initialize()', there shouldn't double
the operations.

The key-point is avoiding TLB/RLB hash table is set to NULL while closing.
As for the last question, I think this issue due to timeline between
close processing and TLB / RLB operating.

If all these resetting operations are in 'bond_close()' instead of
'bond_open()', there still exist the same race conditions.

For example:
bond_close------------bond_start_xmit
spin_lock_bh----------bond_alb_xmit
tlb_init_table_entry--tlb_choose_channel
spin_unlock_bh--------//wait for lock
----------------------spin_lock_bh
----------------------tx_hashtbl[hash_index].tx_slave
----------------------spin_unlock_bh

If 'bond_close()' preempts the lock before TX, it won't be able to choose
the correct tx_slave in 'tlb_choose_channel()'.

However, if all these are in 'bond_open()', when the same race conditions
happen, TX can still work normally.


  Looking forward to your opinion on my reply!

-sujing


>> 		if (bond->params.tlb_dynamic_lb || BOND_MODE(bond) == BOND_MODE_ALB)
>> 			queue_delayed_work(bond->wq, &bond->alb_work, 0);
>> 	}
>> @@ -4279,8 +4298,6 @@ static int bond_close(struct net_device *bond_dev)
>>
>> 	bond_work_cancel_all(bond);
>> 	bond->send_peer_notif = 0;
>> -	if (bond_is_lb(bond))
>> -		bond_alb_deinitialize(bond);
>> 	bond->recv_probe = NULL;
>>
>> 	if (bond_uses_primary(bond)) {
>> @@ -5854,6 +5871,8 @@ static void bond_uninit(struct net_device *bond_dev)
>> 	struct list_head *iter;
>> 	struct slave *slave;
>>
>> +	bond_alb_deinitialize(bond);
>> +
>> 	bond_netpoll_cleanup(bond_dev);
>>
>> 	/* Release the bonded slaves */
>> @@ -6295,6 +6314,12 @@ static int bond_init(struct net_device *bond_dev)
>> 	    bond_dev->addr_assign_type == NET_ADDR_PERM)
>> 		eth_hw_addr_random(bond_dev);
>>
>> +	/* bond_alb_initialize must be called before the timer
>> +	 * is started.
>> +	 */
>> +	if (bond_alb_initialize(bond))
>> +		return -ENOMEM;
>> +
>> 	return 0;
>> }
>>
>> diff --git a/include/net/bond_alb.h b/include/net/bond_alb.h
>> index 9dc082b2d543..9fd16e20ef82 100644
>> --- a/include/net/bond_alb.h
>> +++ b/include/net/bond_alb.h
>> @@ -150,7 +150,7 @@ struct alb_bond_info {
>> 						 */
>> };
>>
>> -int bond_alb_initialize(struct bonding *bond, int rlb_enabled);
>> +int bond_alb_initialize(struct bonding *bond);
>> void bond_alb_deinitialize(struct bonding *bond);
>> int bond_alb_init_slave(struct bonding *bond, struct slave *slave);
>> void bond_alb_deinit_slave(struct bonding *bond, struct slave *slave);
>> @@ -165,5 +165,8 @@ struct slave *bond_xmit_tlb_slave_get(struct bonding *bond,
>> void bond_alb_monitor(struct work_struct *);
>> int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr);
>> void bond_alb_clear_vlan(struct bonding *bond, unsigned short vlan_id);
>> +int rlb_arp_recv(const struct sk_buff *skb, struct bonding *bond, struct slave *slave);
>> +void tlb_init_table_entry(struct tlb_client_info *entry, int save_load);
>> +void rlb_init_table_entry(struct rlb_client_info *entry);
>> #endif /* _NET_BOND_ALB_H */
>>
>> -- 
>> 2.27.0
>>
> ---
> 	-Jay Vosburgh, jay.vosburgh@canonical.com

-- 
sujing <sujing@kylinos.cn>