BUG? possible race due to the absence of barrier

BUG? possible race due to the absence of barrier

[홍신 shin hong]

Hi. I am reporting a type of suspected bugs due to
the lack of enforcing operation order by memory barrier.

I found this issue while I read the code, so that
it might not be real. But, please examine this issue.

We often allocate an object, initialize it, and then link it to a data
structure.
Then any thread can access the object.

For this pattern of programming, it seems to be necessary that
memory barrier should confirm that the initializations and the linking
to global data structures are not disordered by CPU or compilers.

atm_add_addr() in /net/atm/addr.c has the following code:

 88   this = kmalloc(sizeof(struct atm_dev_addr), GFP_ATOMIC);
 89   if (!this) {
 90     spin_unlock_irqrestore(&dev->lock, flags);
 91     return -ENOMEM;
 92   }
 93   this->addr = *addr;
 94   list_add(&this->entry, head);

The operation at line 93 might be executed earlier than that of line 94.
Then, the other thread might read uninitialized value of this
if there is other concurrent thread which iterates the list.

Please examine this issue and let me know your opinions.
Thank you.

Sincerely
Shin Hong

Re: BUG? possible race due to the absence of barrier

[Jarek Poplawski]

On 13-01-2010 05:35, ?? shin hong wrote:
> Hi. I am reporting a type of suspected bugs due to
> the lack of enforcing operation order by memory barrier.
> 
> I found this issue while I read the code, so that
> it might not be real. But, please examine this issue.
> 
> We often allocate an object, initialize it, and then link it to a data
> structure.
> Then any thread can access the object.
> 
> For this pattern of programming, it seems to be necessary that
> memory barrier should confirm that the initializations and the linking
> to global data structures are not disordered by CPU or compilers.
> 
> atm_add_addr() in /net/atm/addr.c has the following code:
> 
>  88   this = kmalloc(sizeof(struct atm_dev_addr), GFP_ATOMIC);
>  89   if (!this) {
>  90     spin_unlock_irqrestore(&dev->lock, flags);
>  91     return -ENOMEM;
>  92   }
>  93   this->addr = *addr;
>  94   list_add(&this->entry, head);
> 
> The operation at line 93 might be executed earlier than that of line 94.
> Then, the other thread might read uninitialized value of this
> if there is other concurrent thread which iterates the list.
> 
> Please examine this issue and let me know your opinions.

Should be safe under the spin_lock() (and its barriers).

Regards,
Jarek P.