[PATCH] Fix Kernel Panic in bonding driver debugfs file: rlb_hash_table

[PATCH] Fix Kernel Panic in bonding driver debugfs file: rlb_hash_table

[Pai]

This patch fixes a Kernel Panic in bonding driver debugfs file: rlb_hash_table.

$> modprobe bonding mode=6
$> cat /sys/kernel/debug/bonding/bond0/rlb_hash_table

This will crash the kernel. The struct alb_bond_info is initialized only when
the bonding interface is initialized (ip link set bond0 up) and not at the time
it is allocated. If we try to read the table before that, it'll result in a 
kernel panic.

The patch applies against both net and net-next

Signed-off-by: Vishwanath Pai <vpai@akamai.com>
 
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 089a402..806892a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4510,6 +4510,8 @@ unsigned int bond_get_num_tx_queues(void)
 int bond_create(struct net *net, const char *name)
 {
 	struct net_device *bond_dev;
+	struct bonding *bond;
+	struct alb_bond_info *bond_info;
 	int res;
 
 	rtnl_lock();
@@ -4523,6 +4525,14 @@ int bond_create(struct net *net, const char *name)
 		return -ENOMEM;
 	}
 
+	/*
+	 * Initialize rx_hashtbl_used_head to RLB_NULL_INDEX.
+	 * It is set to 0 by default which is wrong.
+	 */
+	bond = netdev_priv(bond_dev);
+	bond_info = &(BOND_ALB_INFO(bond));
+	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
+
 	dev_net_set(bond_dev, net);
 	bond_dev->rtnl_link_ops = &bond_link_ops;
 

Re: [PATCH] Fix Kernel Panic in bonding driver debugfs file: rlb_hash_table

[Andy Gospodarek]

On Wed, Apr 29, 2015 at 02:24:23PM -0400, Pai wrote:
> This patch fixes a Kernel Panic in bonding driver debugfs file: rlb_hash_table.
> 
> $> modprobe bonding mode=6
> $> cat /sys/kernel/debug/bonding/bond0/rlb_hash_table
> 
> This will crash the kernel. The struct alb_bond_info is initialized only when
> the bonding interface is initialized (ip link set bond0 up) and not at the time
> it is allocated. If we try to read the table before that, it'll result in a 
> kernel panic.

Nice catch!

> 
> The patch applies against both net and net-next
> 
> Signed-off-by: Vishwanath Pai <vpai@akamai.com>

Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>

>  
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 089a402..806892a 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4510,6 +4510,8 @@ unsigned int bond_get_num_tx_queues(void)
>  int bond_create(struct net *net, const char *name)
>  {
>  	struct net_device *bond_dev;
> +	struct bonding *bond;
> +	struct alb_bond_info *bond_info;
>  	int res;
>  
>  	rtnl_lock();
> @@ -4523,6 +4525,14 @@ int bond_create(struct net *net, const char *name)
>  		return -ENOMEM;
>  	}
>  
> +	/*
> +	 * Initialize rx_hashtbl_used_head to RLB_NULL_INDEX.
> +	 * It is set to 0 by default which is wrong.
> +	 */
> +	bond = netdev_priv(bond_dev);
> +	bond_info = &(BOND_ALB_INFO(bond));
> +	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
> +
>  	dev_net_set(bond_dev, net);
>  	bond_dev->rtnl_link_ops = &bond_link_ops;
>  
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix Kernel Panic in bonding driver debugfs file: rlb_hash_table

[David Miller]

From: Andy Gospodarek <gospo@cumulusnetworks.com>
Date: Wed, 29 Apr 2015 14:51:07 -0400

> On Wed, Apr 29, 2015 at 02:24:23PM -0400, Pai wrote:
>> This patch fixes a Kernel Panic in bonding driver debugfs file: rlb_hash_table.
>> 
>> $> modprobe bonding mode=6
>> $> cat /sys/kernel/debug/bonding/bond0/rlb_hash_table
>> 
>> This will crash the kernel. The struct alb_bond_info is initialized only when
>> the bonding interface is initialized (ip link set bond0 up) and not at the time
>> it is allocated. If we try to read the table before that, it'll result in a 
>> kernel panic.
> 
> Nice catch!
> 
>> 
>> The patch applies against both net and net-next
>> 
>> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
> 
> Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>

Applied, thanks.

Re: [PATCH] Fix Kernel Panic in bonding driver debugfs file: rlb_hash_table

[Nikolay Aleksandrov]

On 04/29/2015 08:24 PM, Pai wrote:
> This patch fixes a Kernel Panic in bonding driver debugfs file: rlb_hash_table.
> 
> $> modprobe bonding mode=6
> $> cat /sys/kernel/debug/bonding/bond0/rlb_hash_table
> 
> This will crash the kernel. The struct alb_bond_info is initialized only when
> the bonding interface is initialized (ip link set bond0 up) and not at the time
> it is allocated. If we try to read the table before that, it'll result in a 
> kernel panic.
> 
> The patch applies against both net and net-next
> 
> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
>  
Good catch, a few cosmetic nits below.

> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 089a402..806892a 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -4510,6 +4510,8 @@ unsigned int bond_get_num_tx_queues(void)
>  int bond_create(struct net *net, const char *name)
>  {
>  	struct net_device *bond_dev;
> +	struct bonding *bond;
> +	struct alb_bond_info *bond_info;
Please arrange these longest to shortest line, it's easier to read.

>  	int res;
>  
>  	rtnl_lock();
> @@ -4523,6 +4525,14 @@ int bond_create(struct net *net, const char *name)
>  		return -ENOMEM;
>  	}
>  
> +	/*
> +	 * Initialize rx_hashtbl_used_head to RLB_NULL_INDEX.
> +	 * It is set to 0 by default which is wrong.
> +	 */
Multiline comment style of networking content is:
	/* text
	 * text
	 */

See: Documentation/networking/netdev-FAQ.txt

Alternatively you can create an inline with descriptive name that does the
initialization and wouldn't need the comment. Either way's fine by me.

Cheers,
 Nik

> +	bond = netdev_priv(bond_dev);
> +	bond_info = &(BOND_ALB_INFO(bond));
> +	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
> +
>  	dev_net_set(bond_dev, net);
>  	bond_dev->rtnl_link_ops = &bond_link_ops;
>  
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] Fix Kernel Panic in bonding driver debugfs file: rlb_hash_table

[Vishwanath Pai]

On 04/29/2015 03:45 PM, Nikolay Aleksandrov wrote:
> On 04/29/2015 08:24 PM, Pai wrote:
>> This patch fixes a Kernel Panic in bonding driver debugfs file: rlb_hash_table.
>>
>> $> modprobe bonding mode=6
>> $> cat /sys/kernel/debug/bonding/bond0/rlb_hash_table
>>
>> This will crash the kernel. The struct alb_bond_info is initialized only when
>> the bonding interface is initialized (ip link set bond0 up) and not at the time
>> it is allocated. If we try to read the table before that, it'll result in a 
>> kernel panic.
>>
>> The patch applies against both net and net-next
>>
>> Signed-off-by: Vishwanath Pai <vpai@akamai.com>
>>  
> Good catch, a few cosmetic nits below.
> 
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 089a402..806892a 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -4510,6 +4510,8 @@ unsigned int bond_get_num_tx_queues(void)
>>  int bond_create(struct net *net, const char *name)
>>  {
>>  	struct net_device *bond_dev;
>> +	struct bonding *bond;
>> +	struct alb_bond_info *bond_info;
> Please arrange these longest to shortest line, it's easier to read.
> 
>>  	int res;
>>  
>>  	rtnl_lock();
>> @@ -4523,6 +4525,14 @@ int bond_create(struct net *net, const char *name)
>>  		return -ENOMEM;
>>  	}
>>  
>> +	/*
>> +	 * Initialize rx_hashtbl_used_head to RLB_NULL_INDEX.
>> +	 * It is set to 0 by default which is wrong.
>> +	 */
> Multiline comment style of networking content is:
> 	/* text
> 	 * text
> 	 */
> 
> See: Documentation/networking/netdev-FAQ.txt
> 
> Alternatively you can create an inline with descriptive name that does the
> initialization and wouldn't need the comment. Either way's fine by me.
> 
> Cheers,
>  Nik
> 
>> +	bond = netdev_priv(bond_dev);
>> +	bond_info = &(BOND_ALB_INFO(bond));
>> +	bond_info->rx_hashtbl_used_head = RLB_NULL_INDEX;
>> +
>>  	dev_net_set(bond_dev, net);
>>  	bond_dev->rtnl_link_ops = &bond_link_ops;
>>  
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

David,

Since the patch has been applied already - should I send a V2 with the
suggested changes? Or submit a new patch that applies on top of the
first patch?

Thanks,
Vishwanath

Re: [PATCH] Fix Kernel Panic in bonding driver debugfs file: rlb_hash_table

[David Miller]

From: Vishwanath Pai <vpai@akamai.com>
Date: Wed, 29 Apr 2015 16:30:44 -0400

> Since the patch has been applied already - should I send a V2 with the
> suggested changes? Or submit a new patch that applies on top of the
> first patch?

You should always send me a relative patch in this situation.