question about fs/ubifs/orphan.c

question about fs/ubifs/orphan.c

[Julia Lawall]

The function ubifs_orphan_start_commit contains the code:

         list_for_each_entry(orphan, &c->orph_new, new_list) {
                 ubifs_assert(orphan->new);
                 orphan->new = 0;
                 *last = orphan;
                 last = &orphan->cnext;
         }
         *last = orphan->cnext;

After list_for_each_entry, orphan is just an address at an offset from the 
list head, not a pointer to a real structure.  So it does not seem correct 
to access its cnext field.

julia

Re: question about fs/ubifs/orphan.c

[Julia Lawall]

There is another occurrence of the same pattern in the function 
consolidate in the same file.

julia

On Sun, 8 Jul 2012, Julia Lawall wrote:

> The function ubifs_orphan_start_commit contains the code:
>
>        list_for_each_entry(orphan, &c->orph_new, new_list) {
>                ubifs_assert(orphan->new);
>                orphan->new = 0;
>                *last = orphan;
>                last = &orphan->cnext;
>        }
>        *last = orphan->cnext;
>
> After list_for_each_entry, orphan is just an address at an offset from the 
> list head, not a pointer to a real structure.  So it does not seem correct 
> to access its cnext field.
>
> julia
>

Re: question about fs/ubifs/orphan.c

[Adrian Hunter]

On 08/07/12 12:22, Julia Lawall wrote:
> There is another occurrence of the same pattern in the function consolidate
> in the same file.

Yes. It needs the same fix.  Will you send a patch?

> 
> julia
> 
> On Sun, 8 Jul 2012, Julia Lawall wrote:
> 
>> The function ubifs_orphan_start_commit contains the code:
>>
>>        list_for_each_entry(orphan, &c->orph_new, new_list) {
>>                ubifs_assert(orphan->new);
>>                orphan->new = 0;
>>                *last = orphan;
>>                last = &orphan->cnext;
>>        }
>>        *last = orphan->cnext;
>>
>> After list_for_each_entry, orphan is just an address at an offset from the
>> list head, not a pointer to a real structure.  So it does not seem correct
>> to access its cnext field.
>>
>> julia
>>
> 
> 

Re: question about fs/ubifs/orphan.c

[Julia Lawall]

On Mon, 9 Jul 2012, Adrian Hunter wrote:

> On 08/07/12 12:22, Julia Lawall wrote:
>> There is another occurrence of the same pattern in the function consolidate
>> in the same file.
>
> Yes. It needs the same fix.  Will you send a patch?

Oops, I seem to have overlooked it again.  I will send a patch that fixes 
both.

julia

>>
>> julia
>>
>> On Sun, 8 Jul 2012, Julia Lawall wrote:
>>
>>> The function ubifs_orphan_start_commit contains the code:
>>>
>>>        list_for_each_entry(orphan, &c->orph_new, new_list) {
>>>                ubifs_assert(orphan->new);
>>>                orphan->new = 0;
>>>                *last = orphan;
>>>                last = &orphan->cnext;
>>>        }
>>>        *last = orphan->cnext;
>>>
>>> After list_for_each_entry, orphan is just an address at an offset from the
>>> list head, not a pointer to a real structure.  So it does not seem correct
>>> to access its cnext field.
>>>
>>> julia
>>>
>>
>>
>
>
>

Re: question about fs/ubifs/orphan.c

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Sun, 2012-07-08 at 11:21 +0200, Julia Lawall wrote:
> The function ubifs_orphan_start_commit contains the code:
> 
>          list_for_each_entry(orphan, &c->orph_new, new_list) {
>                  ubifs_assert(orphan->new);
>                  orphan->new = 0;
>                  *last = orphan;
>                  last = &orphan->cnext;
>          }
>          *last = orphan->cnext;
> 
> After list_for_each_entry, orphan is just an address at an offset from the 
> list head, not a pointer to a real structure.  So it does not seem correct 
> to access its cnext field.

Looks like you've spotted a but - we write some irrelevant address to an
area within the 'struct ubifs_info'.

I think what the code meant to do is to write NULL there:

- *last = orphan->cnext;
+ *last = NULL;

I wonder if this could be a reason for some of strange bugs we have seen
reports for.

Well-spotted Julia, thanks! How did you do this - writing another cocci
script for the kernel?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: question about fs/ubifs/orphan.c

[Julia Lawall]

On Sun, 8 Jul 2012, Artem Bityutskiy wrote:

> On Sun, 2012-07-08 at 11:21 +0200, Julia Lawall wrote:
>> The function ubifs_orphan_start_commit contains the code:
>>
>>          list_for_each_entry(orphan, &c->orph_new, new_list) {
>>                  ubifs_assert(orphan->new);
>>                  orphan->new = 0;
>>                  *last = orphan;
>>                  last = &orphan->cnext;
>>          }
>>          *last = orphan->cnext;
>>
>> After list_for_each_entry, orphan is just an address at an offset from the
>> list head, not a pointer to a real structure.  So it does not seem correct
>> to access its cnext field.
>
> Looks like you've spotted a but - we write some irrelevant address to an
> area within the 'struct ubifs_info'.
>
> I think what the code meant to do is to write NULL there:
>
> - *last = orphan->cnext;
> + *last = NULL;
>
> I wonder if this could be a reason for some of strange bugs we have seen
> reports for.
>
> Well-spotted Julia, thanks! How did you do this - writing another cocci
> script for the kernel?

Yes: http://lkml.indiana.edu/hypermail/linux/kernel/1207.1/00028.html

I'll send a patch.

julia

Re: question about fs/ubifs/orphan.c

[Adrian Hunter]

On 08/07/12 15:40, Artem Bityutskiy wrote:
> On Sun, 2012-07-08 at 11:21 +0200, Julia Lawall wrote:
>> The function ubifs_orphan_start_commit contains the code:
>>
>>          list_for_each_entry(orphan, &c->orph_new, new_list) {
>>                  ubifs_assert(orphan->new);
>>                  orphan->new = 0;
>>                  *last = orphan;
>>                  last = &orphan->cnext;
>>          }
>>          *last = orphan->cnext;
>>
>> After list_for_each_entry, orphan is just an address at an offset from the 
>> list head, not a pointer to a real structure.  So it does not seem correct 
>> to access its cnext field.
> 
> Looks like you've spotted a but - we write some irrelevant address to an
> area within the 'struct ubifs_info'.
> 
> I think what the code meant to do is to write NULL there:
> 
> - *last = orphan->cnext;
> + *last = NULL;
> 
> I wonder if this could be a reason for some of strange bugs we have seen
> reports for.

Perhaps not because in that case c->cmt_orphans is zero so the cnext list
is never dereferenced.

> 
> Well-spotted Julia, thanks! How did you do this - writing another cocci
> script for the kernel?
>