* Re: net_tx_action race condition?
[not found] <BY2PR05MB712796CB064F3202FEFA328B3A30@BY2PR05MB712.namprd05.prod.outlook.com>
@ 2018-03-28 16:32 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2018-03-28 16:32 UTC (permalink / raw)
To: Saurabh Kr, Angelo Rizzi
Cc: netdev, linux-kernel, Sarvendra Vikram Singh, Kunal Sharma
On 03/28/2018 12:30 AM, Saurabh Kr wrote:
> Hi Eric/Angelo,
>
> We are seeing the assertion error in linux kernel 2.4.29 “*kernel: KERNEL: assertion (atomic_read(&skb->users) == 0) failed at dev.c(1397)**”.* Based on patch provided (_https://patchwork.kernel.org/patch/5368051/_ ) we merged the changes in linux kernel 2.4.29 but we are still facing the assertion error at dev.c (1397). Please let me know your thoughts.
>
> *Before Merge**(linux 2.4.29)*
> ---------------------------------
>
> static void net_tx_action(struct softirq_action *h)
> {
> int cpu = smp_processor_id();
>
> if (softnet_data[cpu].completion_queue) {
> struct sk_buff *clist;
>
> local_irq_disable();
> clist = softnet_data[cpu].completion_queue; // Existing code
> softnet_data[cpu].completion_queue = NULL;
> local_irq_enable();
>
> while (clist != NULL) {
> struct sk_buff *skb = clist;
> clist = clist->next;
>
> BUG_TRAP(atomic_read(&skb->users) == 0);
> __kfree_skb(skb);
> }
> }
>
> ---------
>
> *After Merge the changes based on available patch**(linux 2.4.29)**:*
> ------------------------------------------------------------------------------
>
> static void net_tx_action(struct softirq_action *h)
> {
> int cpu = smp_processor_id();
>
> if (softnet_data[cpu].completion_queue) {
> struct sk_buff *clist;
>
> local_irq_disable();
> clist = *(volatile typeof(softnet_data[cpu].completion_queue) *)&( softnet_data[cpu].completion_queue); // Modified line based on available patch
> softnet_data[cpu].completion_queue = NULL;
> local_irq_enable();
>
> while (clist != NULL) {
> struct sk_buff *skb = clist;
> clist = clist->next;
>
> BUG_TRAP(atomic_read(&skb->users) == 0);
> __kfree_skb(skb);
> }
> }
> ………….
>
> Thanks & regards,
> Saurabh
>
Thats simply prove (again) that this 'fix' was not the proper one.
I have no idea what is wrong, and there is no way I am going to look at 2.4.29 kernel...
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: net_tx_action race condition?
2014-11-24 15:33 ` Eric Dumazet
@ 2014-11-24 17:19 ` Angelo Rizzi
0 siblings, 0 replies; 5+ messages in thread
From: Angelo Rizzi @ 2014-11-24 17:19 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, linux-kernel
Hi Eric,
Thanks for your reply
You are right, it seems a bug in the NIOS2 architecture port.
I will check how local_irq_disable()/local_irq_enable() is implemented
on this kind of architecture.
Regards,
Angelo
Il 24/11/2014 16:33, Eric Dumazet ha scritto:
> On Mon, 2014-11-24 at 14:29 +0100, Angelo Rizzi wrote:
>> Hi Daniel,
>> Here attached the patch file you required.
>> The problem i've found is on the declaration of 'struct softnet_data
>> *sd' in function 'net_tx_action'
>> What happens to me (i have an embedded system based on FPGA and a NIOS2
>> cpu) is that, due to compiler optimization, the content of
>> 'sd->completion_queue' is saved in a CPU register before interrupt
>> disabling (when the instruction 'if (sd->completion_queue) {' is
>> executed) and then the register contents is used for interrupt-disabled
>> assignment ('clist = sd->completion_queue') instead of re-read the
>> variable contents.
>> This seems to lead to a race condition when an interrupt modifies the
>> content of 'sd->completion_queue' between these two instructions.
>> What i have done to avoid this situation is to change the declaration of
>> 'struct softnet_data *sd' to 'volatile struct softnet_data *sd' and now
>> everything seems to be ok.
>> I hope this will help.
>>
>> Regards,
>> Angelo
>>
> Do not add volatile in the kernel, this is not how we solve this kind of
> problems. Documentation/memory-barriers.txt and
>
> Documentation/00-INDEX:476:volatile-considered-harmful.txt
> Documentation/00-INDEX:477: - Why the "volatile" type class should not be used
>
>
> I am surprised this patch is needed. Many other 'bugs' would need
> similar fixes.
>
> local_irq_disable() MUST have a memory barrier. This looks like a bug in
> one particular arch implementation.
>
>
> On x86 for example, native_irq_disable() is really :
>
> asm volatile("cli": : :"memory");
>
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ac4836241a965a71952469ba054f87d8dfca2d32..fa73072e515aa07fa8ae1bc39174b7d59c7a31a5 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3406,7 +3406,7 @@ static void net_tx_action(struct softirq_action *h)
> struct sk_buff *clist;
>
> local_irq_disable();
> - clist = sd->completion_queue;
> + clist = ACCESS_ONCE(sd->completion_queue);
> sd->completion_queue = NULL;
> local_irq_enable();
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: net_tx_action race condition?
2014-11-24 13:29 Angelo Rizzi
2014-11-24 15:08 ` Herbert Xu
@ 2014-11-24 15:33 ` Eric Dumazet
2014-11-24 17:19 ` Angelo Rizzi
1 sibling, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2014-11-24 15:33 UTC (permalink / raw)
To: Angelo Rizzi; +Cc: netdev, linux-kernel
On Mon, 2014-11-24 at 14:29 +0100, Angelo Rizzi wrote:
> Hi Daniel,
> Here attached the patch file you required.
> The problem i've found is on the declaration of 'struct softnet_data
> *sd' in function 'net_tx_action'
> What happens to me (i have an embedded system based on FPGA and a NIOS2
> cpu) is that, due to compiler optimization, the content of
> 'sd->completion_queue' is saved in a CPU register before interrupt
> disabling (when the instruction 'if (sd->completion_queue) {' is
> executed) and then the register contents is used for interrupt-disabled
> assignment ('clist = sd->completion_queue') instead of re-read the
> variable contents.
> This seems to lead to a race condition when an interrupt modifies the
> content of 'sd->completion_queue' between these two instructions.
> What i have done to avoid this situation is to change the declaration of
> 'struct softnet_data *sd' to 'volatile struct softnet_data *sd' and now
> everything seems to be ok.
> I hope this will help.
>
> Regards,
> Angelo
>
Do not add volatile in the kernel, this is not how we solve this kind of
problems. Documentation/memory-barriers.txt and
Documentation/00-INDEX:476:volatile-considered-harmful.txt
Documentation/00-INDEX:477: - Why the "volatile" type class should not be used
I am surprised this patch is needed. Many other 'bugs' would need
similar fixes.
local_irq_disable() MUST have a memory barrier. This looks like a bug in
one particular arch implementation.
On x86 for example, native_irq_disable() is really :
asm volatile("cli": : :"memory");
diff --git a/net/core/dev.c b/net/core/dev.c
index ac4836241a965a71952469ba054f87d8dfca2d32..fa73072e515aa07fa8ae1bc39174b7d59c7a31a5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3406,7 +3406,7 @@ static void net_tx_action(struct softirq_action *h)
struct sk_buff *clist;
local_irq_disable();
- clist = sd->completion_queue;
+ clist = ACCESS_ONCE(sd->completion_queue);
sd->completion_queue = NULL;
local_irq_enable();
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: net_tx_action race condition?
2014-11-24 13:29 Angelo Rizzi
@ 2014-11-24 15:08 ` Herbert Xu
2014-11-24 15:33 ` Eric Dumazet
1 sibling, 0 replies; 5+ messages in thread
From: Herbert Xu @ 2014-11-24 15:08 UTC (permalink / raw)
To: Angelo Rizzi; +Cc: netdev
Angelo Rizzi <angelo.rizzi@3dautomazione.it> wrote:
> [-- text/plain, encoding 7bit, charset: iso-8859-15, 22 lines --]
>
> Hi Daniel,
> Here attached the patch file you required.
> The problem i've found is on the declaration of 'struct softnet_data
> *sd' in function 'net_tx_action'
> What happens to me (i have an embedded system based on FPGA and a NIOS2
> cpu) is that, due to compiler optimization, the content of
> 'sd->completion_queue' is saved in a CPU register before interrupt
> disabling (when the instruction 'if (sd->completion_queue) {' is
> executed) and then the register contents is used for interrupt-disabled
> assignment ('clist = sd->completion_queue') instead of re-read the
> variable contents.
> This seems to lead to a race condition when an interrupt modifies the
> content of 'sd->completion_queue' between these two instructions.
> What i have done to avoid this situation is to change the declaration of
> 'struct softnet_data *sd' to 'volatile struct softnet_data *sd' and now
> everything seems to be ok.
> I hope this will help.
local_irq_disable is supposed to contain a compiler barrier. So
this is either a buggy arch or a buggy compiler.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 5+ messages in thread
* net_tx_action race condition?
@ 2014-11-24 13:29 Angelo Rizzi
2014-11-24 15:08 ` Herbert Xu
2014-11-24 15:33 ` Eric Dumazet
0 siblings, 2 replies; 5+ messages in thread
From: Angelo Rizzi @ 2014-11-24 13:29 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 936 bytes --]
Hi Daniel,
Here attached the patch file you required.
The problem i've found is on the declaration of 'struct softnet_data
*sd' in function 'net_tx_action'
What happens to me (i have an embedded system based on FPGA and a NIOS2
cpu) is that, due to compiler optimization, the content of
'sd->completion_queue' is saved in a CPU register before interrupt
disabling (when the instruction 'if (sd->completion_queue) {' is
executed) and then the register contents is used for interrupt-disabled
assignment ('clist = sd->completion_queue') instead of re-read the
variable contents.
This seems to lead to a race condition when an interrupt modifies the
content of 'sd->completion_queue' between these two instructions.
What i have done to avoid this situation is to change the declaration of
'struct softnet_data *sd' to 'volatile struct softnet_data *sd' and now
everything seems to be ok.
I hope this will help.
Regards,
Angelo
[-- Attachment #2: net_tx_action_patch --]
[-- Type: text/plain, Size: 438 bytes --]
--- linux-2.6.32.64/net/core/dev.c.orig 2014-11-24 12:52:36.103335551 +0100
+++ linux-2.6.32.64/net/core/dev.c 2014-11-24 12:52:58.863949715 +0100
@@ -2069,7 +2069,7 @@ EXPORT_SYMBOL(netif_rx_ni);
static void net_tx_action(struct softirq_action *h)
{
- struct softnet_data *sd = &__get_cpu_var(softnet_data);
+ volatile struct softnet_data *sd = &__get_cpu_var(softnet_data);
if (sd->completion_queue) {
struct sk_buff *clist;
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-28 16:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <BY2PR05MB712796CB064F3202FEFA328B3A30@BY2PR05MB712.namprd05.prod.outlook.com>
2018-03-28 16:32 ` net_tx_action race condition? Eric Dumazet
2014-11-24 13:29 Angelo Rizzi
2014-11-24 15:08 ` Herbert Xu
2014-11-24 15:33 ` Eric Dumazet
2014-11-24 17:19 ` Angelo Rizzi
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.