xfs_bmap_add_extent_delay_real: Uninited r[3] corrupts startoff

xfs_bmap_add_extent_delay_real: Uninited r[3] corrupts startoff

[Shailendra Tripathi]

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

Hi,
     It appears that uninitialized r[3] in 
xfs_bmap_add_extent_delay_real can potentially corrupt the startoff for 
a particular case.

This sequence is below:

        xfs_bmap_add_extent_delay_real (
        ...
        xfs_bmbt_irec_t         r[3];   /* neighbor extent entries */

case 0:
          /*
           * Filling in the middle part of a previous delayed allocation.
           * Contiguity is impossible here.
           * This case is avoided almost all the time.
           */
temp = new->br_startoff - PREV.br_startoff;
xfs_bmbt_set_blockcount(ep, temp);
r[0] = *new;
r[1].br_startoff = new_endoff;
temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
r[1].br_blockcount = temp2;
xfs_bmap_insert_exlist(ip, idx + 1, 2, &r[0], XFS_DATA_FORK);
ip->i_df.if_lastex = idx + 1;
ip->i_d.di_nextents++;

Look at extent r[1]. It does not set br_startblock. That is, it is any 
random value. Now, look at the xfs_bmbt_set_all. Though, it sets the 
blockcount later, the startoff does not get changed.

#if XFS_BIG_BLKNOS
         ASSERT((s->br_startblock & XFS_MASK64HI(12)) == 0);
         r->l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
                  ((xfs_bmbt_rec_base_t)s->br_startoff << 9) |
                  ((xfs_bmbt_rec_base_t)s->br_startblock >> 43);
Top 21 bits are taken as it is. However, only 9 bit should be taken. So, 
for random values, it corrupts the startoff which from 9-63 bits.

         r->l1 = ((xfs_bmbt_rec_base_t)s->br_startblock << 21) |
                  ((xfs_bmbt_rec_base_t)s->br_blockcount &
                  (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));

I have attached a small program which does the same thing as it is being 
done here. I would appreciate if someone can verify that assertion is 
correct.


Regards,
Shailendra

[-- Attachment #2: del_bmap.c --]
[-- Type: text/plain, Size: 1674 bytes --]

#include <stdio.h>
typedef unsigned long __uint64_t;
typedef struct xfs_bmbt_rec_64
{
        __uint64_t              l0, l1;
} xfs_bmbt_rec_64_t;

typedef __uint64_t      xfs_bmbt_rec_base_t;    
typedef xfs_bmbt_rec_64_t xfs_bmbt_rec_t, xfs_bmdr_rec_t;

typedef enum {
       XFS_EXT_NORM, XFS_EXT_UNWRITTEN,
       XFS_EXT_DMAPI_OFFLINE
} xfs_exntst_t;

typedef struct xfs_bmbt_irec
{
	__uint64_t   br_startoff;    /* starting file offset */
	__uint64_t   br_startblock;  /* starting block number */
	__uint64_t   br_blockcount;  /* number of blocks */
	xfs_exntst_t    br_state;       /* extent state */
} xfs_bmbt_irec_t;

#define XFS_MASK64LO(n)         (((__uint64_t)1 << (n)) - 1)
#define XFS_MASK64HI(n)         ((__uint64_t)-1 << (64 - (n)))

int main(void) 
{
	xfs_bmbt_irec_t s;
	xfs_bmbt_rec_t  r;
	int extent_flag;

	s.br_startoff = 0;
	s.br_blockcount = 5;
	s.br_startblock = 0xfffffffffffffff0;
	extent_flag = (s.br_state == XFS_EXT_NORM) ? 0 : 1;

	printf("blockcount = 0x%llx\n", s.br_startblock);
	r.l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
	          ((xfs_bmbt_rec_base_t)s.br_startoff << 9) |
	          ((xfs_bmbt_rec_base_t)s.br_startblock >> 43);
	r.l1 = ((xfs_bmbt_rec_base_t)s.br_startblock << 21) |
	         ((xfs_bmbt_rec_base_t)s.br_blockcount &
	         (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));

	printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);

	r.l0 = (r.l0 & (xfs_bmbt_rec_base_t)XFS_MASK64HI(55)) |
	        (xfs_bmbt_rec_base_t)((__uint64_t)100 >> 43);
	r.l1 = (r.l1 & (xfs_bmbt_rec_base_t)XFS_MASK64LO(21)) |
	        (xfs_bmbt_rec_base_t)((__uint64_t)100 << 21);

	printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
	return 0;
}

Re: xfs_bmap_add_extent_delay_real: Uninited r[3] corrupts startoff

[Vlad Apostolov]

Hi Shailendra,

Shailendra Tripathi wrote:
> Hi,
>     It appears that uninitialized r[3] in 
> xfs_bmap_add_extent_delay_real can potentially corrupt the startoff 
> for a particular case.
>
> This sequence is below:
>
>        xfs_bmap_add_extent_delay_real (
>        ...
>        xfs_bmbt_irec_t         r[3];   /* neighbor extent entries */
>
> case 0:
>          /*
>           * Filling in the middle part of a previous delayed allocation.
>           * Contiguity is impossible here.
>           * This case is avoided almost all the time.
>           */
> temp = new->br_startoff - PREV.br_startoff;
> xfs_bmbt_set_blockcount(ep, temp);
> r[0] = *new;
> r[1].br_startoff = new_endoff;
> temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
> r[1].br_blockcount = temp2;
> xfs_bmap_insert_exlist(ip, idx + 1, 2, &r[0], XFS_DATA_FORK);
> ip->i_df.if_lastex = idx + 1;
> ip->i_d.di_nextents++;
>
> Look at extent r[1]. It does not set br_startblock. That is, it is any 
> random value. Now, look at the xfs_bmbt_set_all. Though, it sets the 
> blockcount later, the startoff does not get changed.
>
> #if XFS_BIG_BLKNOS
>         ASSERT((s->br_startblock & XFS_MASK64HI(12)) == 0);
>         r->l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
>                  ((xfs_bmbt_rec_base_t)s->br_startoff << 9) |
>                  ((xfs_bmbt_rec_base_t)s->br_startblock >> 43);
> Top 21 bits are taken as it is. However, only 9 bit should be taken. 
> So, for random values, it corrupts the startoff which from 9-63 bits.
 From the code inspection I agree with you that br_startblock doesn't 
appear
to be initialized in this scenario. Otherwise I think the code looks good.
If the br_startblock is initialized  it should be a value that fits
in 52 bits out of 64 (this is what the ASSERT is for) and the top 12 
bits will be 0.
The r->l0 gets the top 21 bits of br_startblock, the most significant 12 
bits of
which are 0 and least significant 9 could be non 0. The r->l1 gets the
rest 43 (= 52-9 = 64-21) bits of br_startblock.

I will open a bug report for the uninitialized br_startblock.

Thank you for finding this problem.

Regards,
Vlad

>
>         r->l1 = ((xfs_bmbt_rec_base_t)s->br_startblock << 21) |
>                  ((xfs_bmbt_rec_base_t)s->br_blockcount &
>                  (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
>
> I have attached a small program which does the same thing as it is 
> being done here. I would appreciate if someone can verify that 
> assertion is correct.
>
>
> Regards,
> Shailendra
> ------------------------------------------------------------------------
>
> #include <stdio.h>
> typedef unsigned long __uint64_t;
> typedef struct xfs_bmbt_rec_64
> {
>         __uint64_t              l0, l1;
> } xfs_bmbt_rec_64_t;
>
> typedef __uint64_t      xfs_bmbt_rec_base_t;    
> typedef xfs_bmbt_rec_64_t xfs_bmbt_rec_t, xfs_bmdr_rec_t;
>
> typedef enum {
>        XFS_EXT_NORM, XFS_EXT_UNWRITTEN,
>        XFS_EXT_DMAPI_OFFLINE
> } xfs_exntst_t;
>
> typedef struct xfs_bmbt_irec
> {
> 	__uint64_t   br_startoff;    /* starting file offset */
> 	__uint64_t   br_startblock;  /* starting block number */
> 	__uint64_t   br_blockcount;  /* number of blocks */
> 	xfs_exntst_t    br_state;       /* extent state */
> } xfs_bmbt_irec_t;
>
> #define XFS_MASK64LO(n)         (((__uint64_t)1 << (n)) - 1)
> #define XFS_MASK64HI(n)         ((__uint64_t)-1 << (64 - (n)))
>
> int main(void) 
> {
> 	xfs_bmbt_irec_t s;
> 	xfs_bmbt_rec_t  r;
> 	int extent_flag;
>
> 	s.br_startoff = 0;
> 	s.br_blockcount = 5;
> 	s.br_startblock = 0xfffffffffffffff0;
> 	extent_flag = (s.br_state == XFS_EXT_NORM) ? 0 : 1;
>
> 	printf("blockcount = 0x%llx\n", s.br_startblock);
> 	r.l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
> 	          ((xfs_bmbt_rec_base_t)s.br_startoff << 9) |
> 	          ((xfs_bmbt_rec_base_t)s.br_startblock >> 43);
> 	r.l1 = ((xfs_bmbt_rec_base_t)s.br_startblock << 21) |
> 	         ((xfs_bmbt_rec_base_t)s.br_blockcount &
> 	         (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
>
> 	printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
>
> 	r.l0 = (r.l0 & (xfs_bmbt_rec_base_t)XFS_MASK64HI(55)) |
> 	        (xfs_bmbt_rec_base_t)((__uint64_t)100 >> 43);
> 	r.l1 = (r.l1 & (xfs_bmbt_rec_base_t)XFS_MASK64LO(21)) |
> 	        (xfs_bmbt_rec_base_t)((__uint64_t)100 << 21);
>
> 	printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
> 	return 0;
> }
>   

Re: xfs_bmap_add_extent_delay_real: Uninited r[3] corrupts startoff

[Lachlan McIlroy]

This should be all that's needed.  This code handles the case where the middle
portion of a delayed allocation is being converted and splits the extent into
three.  The r[1] extent is the rightmost extent that will remain a delayed
allocation.  Both br_startblock and br_state need to be setup and they will be
the same as the original delayed allocation (PREV) so we just inherit those
values.  Comments?

--- fs/xfs/xfs_bmap.c_1.358     2006-11-01 14:44:38.000000000 +0000
+++ fs/xfs/xfs_bmap.c   2006-11-02 13:22:41.000000000 +0000
@@ -1171,6 +1171,7 @@
                 xfs_bmap_trace_pre_update(fname, "0", ip, idx, XFS_DATA_FORK);
                 xfs_bmbt_set_blockcount(ep, temp);
                 r[0] = *new;
+               r[1] = PREV;
                 r[1].br_startoff = new_endoff;
                 temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
                 r[1].br_blockcount = temp2;

Lachlan

Vlad Apostolov wrote:
> Hi Shailendra,
> 
> Shailendra Tripathi wrote:
> 
>> Hi,
>>     It appears that uninitialized r[3] in 
>> xfs_bmap_add_extent_delay_real can potentially corrupt the startoff 
>> for a particular case.
>>
>> This sequence is below:
>>
>>        xfs_bmap_add_extent_delay_real (
>>        ...
>>        xfs_bmbt_irec_t         r[3];   /* neighbor extent entries */
>>
>> case 0:
>>          /*
>>           * Filling in the middle part of a previous delayed allocation.
>>           * Contiguity is impossible here.
>>           * This case is avoided almost all the time.
>>           */
>> temp = new->br_startoff - PREV.br_startoff;
>> xfs_bmbt_set_blockcount(ep, temp);
>> r[0] = *new;
>> r[1].br_startoff = new_endoff;
>> temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
>> r[1].br_blockcount = temp2;
>> xfs_bmap_insert_exlist(ip, idx + 1, 2, &r[0], XFS_DATA_FORK);
>> ip->i_df.if_lastex = idx + 1;
>> ip->i_d.di_nextents++;
>>
>> Look at extent r[1]. It does not set br_startblock. That is, it is any 
>> random value. Now, look at the xfs_bmbt_set_all. Though, it sets the 
>> blockcount later, the startoff does not get changed.
>>
>> #if XFS_BIG_BLKNOS
>>         ASSERT((s->br_startblock & XFS_MASK64HI(12)) == 0);
>>         r->l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
>>                  ((xfs_bmbt_rec_base_t)s->br_startoff << 9) |
>>                  ((xfs_bmbt_rec_base_t)s->br_startblock >> 43);
>> Top 21 bits are taken as it is. However, only 9 bit should be taken. 
>> So, for random values, it corrupts the startoff which from 9-63 bits.
> 
>  From the code inspection I agree with you that br_startblock doesn't 
> appear
> to be initialized in this scenario. Otherwise I think the code looks good.
> If the br_startblock is initialized  it should be a value that fits
> in 52 bits out of 64 (this is what the ASSERT is for) and the top 12 
> bits will be 0.
> The r->l0 gets the top 21 bits of br_startblock, the most significant 12 
> bits of
> which are 0 and least significant 9 could be non 0. The r->l1 gets the
> rest 43 (= 52-9 = 64-21) bits of br_startblock.
> 
> I will open a bug report for the uninitialized br_startblock.
> 
> Thank you for finding this problem.
> 
> Regards,
> Vlad
> 
>>
>>         r->l1 = ((xfs_bmbt_rec_base_t)s->br_startblock << 21) |
>>                  ((xfs_bmbt_rec_base_t)s->br_blockcount &
>>                  (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
>>
>> I have attached a small program which does the same thing as it is 
>> being done here. I would appreciate if someone can verify that 
>> assertion is correct.
>>
>>
>> Regards,
>> Shailendra
>> ------------------------------------------------------------------------
>>
>> #include <stdio.h>
>> typedef unsigned long __uint64_t;
>> typedef struct xfs_bmbt_rec_64
>> {
>>         __uint64_t              l0, l1;
>> } xfs_bmbt_rec_64_t;
>>
>> typedef __uint64_t      xfs_bmbt_rec_base_t;    typedef 
>> xfs_bmbt_rec_64_t xfs_bmbt_rec_t, xfs_bmdr_rec_t;
>>
>> typedef enum {
>>        XFS_EXT_NORM, XFS_EXT_UNWRITTEN,
>>        XFS_EXT_DMAPI_OFFLINE
>> } xfs_exntst_t;
>>
>> typedef struct xfs_bmbt_irec
>> {
>>     __uint64_t   br_startoff;    /* starting file offset */
>>     __uint64_t   br_startblock;  /* starting block number */
>>     __uint64_t   br_blockcount;  /* number of blocks */
>>     xfs_exntst_t    br_state;       /* extent state */
>> } xfs_bmbt_irec_t;
>>
>> #define XFS_MASK64LO(n)         (((__uint64_t)1 << (n)) - 1)
>> #define XFS_MASK64HI(n)         ((__uint64_t)-1 << (64 - (n)))
>>
>> int main(void) {
>>     xfs_bmbt_irec_t s;
>>     xfs_bmbt_rec_t  r;
>>     int extent_flag;
>>
>>     s.br_startoff = 0;
>>     s.br_blockcount = 5;
>>     s.br_startblock = 0xfffffffffffffff0;
>>     extent_flag = (s.br_state == XFS_EXT_NORM) ? 0 : 1;
>>
>>     printf("blockcount = 0x%llx\n", s.br_startblock);
>>     r.l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
>>               ((xfs_bmbt_rec_base_t)s.br_startoff << 9) |
>>               ((xfs_bmbt_rec_base_t)s.br_startblock >> 43);
>>     r.l1 = ((xfs_bmbt_rec_base_t)s.br_startblock << 21) |
>>              ((xfs_bmbt_rec_base_t)s.br_blockcount &
>>              (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
>>
>>     printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
>>
>>     r.l0 = (r.l0 & (xfs_bmbt_rec_base_t)XFS_MASK64HI(55)) |
>>             (xfs_bmbt_rec_base_t)((__uint64_t)100 >> 43);
>>     r.l1 = (r.l1 & (xfs_bmbt_rec_base_t)XFS_MASK64LO(21)) |
>>             (xfs_bmbt_rec_base_t)((__uint64_t)100 << 21);
>>
>>     printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
>>     return 0;
>> }
>>   
> 
> 
> 

Re: xfs_bmap_add_extent_delay_real: Uninited r[3] corrupts startoff

[Shailendra Tripathi]

Hi Lachlan,
        I would prefer manual assignment here than struct assignment. 
r[1].br_startoff and r[1].br_blockcount will be
modified immediately, so it is not worth assigning via ( r[1] = PREV) as 
it does extra instructions.
Compiler would most likely eliminate the extra assignment but, why to 
leave on the wit of the compiler.

It should be like
r[1].br_state = PREV.br_state;
r[1].br_startblock = 0 ; /* No fancy stuff required here as the aim here 
is that br_startoff does not get any thing random */

Regards,
Shailendra

Lachlan McIlroy wrote:
> This should be all that's needed.  This code handles the case where 
> the middle
> portion of a delayed allocation is being converted and splits the 
> extent into
> three.  The r[1] extent is the rightmost extent that will remain a 
> delayed
> allocation.  Both br_startblock and br_state need to be setup and they 
> will be
> the same as the original delayed allocation (PREV) so we just inherit 
> those
> values.  Comments?
>
> --- fs/xfs/xfs_bmap.c_1.358     2006-11-01 14:44:38.000000000 +0000
> +++ fs/xfs/xfs_bmap.c   2006-11-02 13:22:41.000000000 +0000
> @@ -1171,6 +1171,7 @@
>                 xfs_bmap_trace_pre_update(fname, "0", ip, idx, 
> XFS_DATA_FORK);
>                 xfs_bmbt_set_blockcount(ep, temp);
>                 r[0] = *new;
> +               r[1] = PREV;
>                 r[1].br_startoff = new_endoff;
>                 temp2 = PREV.br_startoff + PREV.br_blockcount - 
> new_endoff;
>                 r[1].br_blockcount = temp2;
>
> Lachlan
>
> Vlad Apostolov wrote:
>> Hi Shailendra,
>>
>> Shailendra Tripathi wrote:
>>
>>> Hi,
>>>     It appears that uninitialized r[3] in 
>>> xfs_bmap_add_extent_delay_real can potentially corrupt the startoff 
>>> for a particular case.
>>>
>>> This sequence is below:
>>>
>>>        xfs_bmap_add_extent_delay_real (
>>>        ...
>>>        xfs_bmbt_irec_t         r[3];   /* neighbor extent entries */
>>>
>>> case 0:
>>>          /*
>>>           * Filling in the middle part of a previous delayed 
>>> allocation.
>>>           * Contiguity is impossible here.
>>>           * This case is avoided almost all the time.
>>>           */
>>> temp = new->br_startoff - PREV.br_startoff;
>>> xfs_bmbt_set_blockcount(ep, temp);
>>> r[0] = *new;
>>> r[1].br_startoff = new_endoff;
>>> temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
>>> r[1].br_blockcount = temp2;
>>> xfs_bmap_insert_exlist(ip, idx + 1, 2, &r[0], XFS_DATA_FORK);
>>> ip->i_df.if_lastex = idx + 1;
>>> ip->i_d.di_nextents++;
>>>
>>> Look at extent r[1]. It does not set br_startblock. That is, it is 
>>> any random value. Now, look at the xfs_bmbt_set_all. Though, it sets 
>>> the blockcount later, the startoff does not get changed.
>>>
>>> #if XFS_BIG_BLKNOS
>>>         ASSERT((s->br_startblock & XFS_MASK64HI(12)) == 0);
>>>         r->l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
>>>                  ((xfs_bmbt_rec_base_t)s->br_startoff << 9) |
>>>                  ((xfs_bmbt_rec_base_t)s->br_startblock >> 43);
>>> Top 21 bits are taken as it is. However, only 9 bit should be taken. 
>>> So, for random values, it corrupts the startoff which from 9-63 bits.
>>
>>  From the code inspection I agree with you that br_startblock doesn't 
>> appear
>> to be initialized in this scenario. Otherwise I think the code looks 
>> good.
>> If the br_startblock is initialized  it should be a value that fits
>> in 52 bits out of 64 (this is what the ASSERT is for) and the top 12 
>> bits will be 0.
>> The r->l0 gets the top 21 bits of br_startblock, the most significant 
>> 12 bits of
>> which are 0 and least significant 9 could be non 0. The r->l1 gets the
>> rest 43 (= 52-9 = 64-21) bits of br_startblock.
>>
>> I will open a bug report for the uninitialized br_startblock.
>>
>> Thank you for finding this problem.
>>
>> Regards,
>> Vlad
>>
>>>
>>>         r->l1 = ((xfs_bmbt_rec_base_t)s->br_startblock << 21) |
>>>                  ((xfs_bmbt_rec_base_t)s->br_blockcount &
>>>                  (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
>>>
>>> I have attached a small program which does the same thing as it is 
>>> being done here. I would appreciate if someone can verify that 
>>> assertion is correct.
>>>
>>>
>>> Regards,
>>> Shailendra
>>> ------------------------------------------------------------------------ 
>>>
>>>
>>> #include <stdio.h>
>>> typedef unsigned long __uint64_t;
>>> typedef struct xfs_bmbt_rec_64
>>> {
>>>         __uint64_t              l0, l1;
>>> } xfs_bmbt_rec_64_t;
>>>
>>> typedef __uint64_t      xfs_bmbt_rec_base_t;    typedef 
>>> xfs_bmbt_rec_64_t xfs_bmbt_rec_t, xfs_bmdr_rec_t;
>>>
>>> typedef enum {
>>>        XFS_EXT_NORM, XFS_EXT_UNWRITTEN,
>>>        XFS_EXT_DMAPI_OFFLINE
>>> } xfs_exntst_t;
>>>
>>> typedef struct xfs_bmbt_irec
>>> {
>>>     __uint64_t   br_startoff;    /* starting file offset */
>>>     __uint64_t   br_startblock;  /* starting block number */
>>>     __uint64_t   br_blockcount;  /* number of blocks */
>>>     xfs_exntst_t    br_state;       /* extent state */
>>> } xfs_bmbt_irec_t;
>>>
>>> #define XFS_MASK64LO(n)         (((__uint64_t)1 << (n)) - 1)
>>> #define XFS_MASK64HI(n)         ((__uint64_t)-1 << (64 - (n)))
>>>
>>> int main(void) {
>>>     xfs_bmbt_irec_t s;
>>>     xfs_bmbt_rec_t  r;
>>>     int extent_flag;
>>>
>>>     s.br_startoff = 0;
>>>     s.br_blockcount = 5;
>>>     s.br_startblock = 0xfffffffffffffff0;
>>>     extent_flag = (s.br_state == XFS_EXT_NORM) ? 0 : 1;
>>>
>>>     printf("blockcount = 0x%llx\n", s.br_startblock);
>>>     r.l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
>>>               ((xfs_bmbt_rec_base_t)s.br_startoff << 9) |
>>>               ((xfs_bmbt_rec_base_t)s.br_startblock >> 43);
>>>     r.l1 = ((xfs_bmbt_rec_base_t)s.br_startblock << 21) |
>>>              ((xfs_bmbt_rec_base_t)s.br_blockcount &
>>>              (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
>>>
>>>     printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
>>>
>>>     r.l0 = (r.l0 & (xfs_bmbt_rec_base_t)XFS_MASK64HI(55)) |
>>>             (xfs_bmbt_rec_base_t)((__uint64_t)100 >> 43);
>>>     r.l1 = (r.l1 & (xfs_bmbt_rec_base_t)XFS_MASK64LO(21)) |
>>>             (xfs_bmbt_rec_base_t)((__uint64_t)100 << 21);
>>>
>>>     printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
>>>     return 0;
>>> }
>>>   
>>
>>
>>

Re: xfs_bmap_add_extent_delay_real: Uninited r[3] corrupts startoff

[Lachlan McIlroy]

I considered that approach but wasn't keen on setting br_startblock to 0
when it should be NULLSTARTBLOCK.  Subsequent calls to xfs_bmbt_set_all()
handle NULLSTARTBLOCK differently but the net result ends up being the
same and the startblock eventually gets overridden anyway.  I'll go with
your suggestion.

Shailendra Tripathi wrote:
> Hi Lachlan,
>        I would prefer manual assignment here than struct assignment. 
> r[1].br_startoff and r[1].br_blockcount will be
> modified immediately, so it is not worth assigning via ( r[1] = PREV) as 
> it does extra instructions.
> Compiler would most likely eliminate the extra assignment but, why to 
> leave on the wit of the compiler.
> 
> It should be like
> r[1].br_state = PREV.br_state;
> r[1].br_startblock = 0 ; /* No fancy stuff required here as the aim here 
> is that br_startoff does not get any thing random */
> 
> Regards,
> Shailendra
> 
> Lachlan McIlroy wrote:
> 
>> This should be all that's needed.  This code handles the case where 
>> the middle
>> portion of a delayed allocation is being converted and splits the 
>> extent into
>> three.  The r[1] extent is the rightmost extent that will remain a 
>> delayed
>> allocation.  Both br_startblock and br_state need to be setup and they 
>> will be
>> the same as the original delayed allocation (PREV) so we just inherit 
>> those
>> values.  Comments?
>>
>> --- fs/xfs/xfs_bmap.c_1.358     2006-11-01 14:44:38.000000000 +0000
>> +++ fs/xfs/xfs_bmap.c   2006-11-02 13:22:41.000000000 +0000
>> @@ -1171,6 +1171,7 @@
>>                 xfs_bmap_trace_pre_update(fname, "0", ip, idx, 
>> XFS_DATA_FORK);
>>                 xfs_bmbt_set_blockcount(ep, temp);
>>                 r[0] = *new;
>> +               r[1] = PREV;
>>                 r[1].br_startoff = new_endoff;
>>                 temp2 = PREV.br_startoff + PREV.br_blockcount - 
>> new_endoff;
>>                 r[1].br_blockcount = temp2;
>>
>> Lachlan
>>
>> Vlad Apostolov wrote:
>>
>>> Hi Shailendra,
>>>
>>> Shailendra Tripathi wrote:
>>>
>>>> Hi,
>>>>     It appears that uninitialized r[3] in 
>>>> xfs_bmap_add_extent_delay_real can potentially corrupt the startoff 
>>>> for a particular case.
>>>>
>>>> This sequence is below:
>>>>
>>>>        xfs_bmap_add_extent_delay_real (
>>>>        ...
>>>>        xfs_bmbt_irec_t         r[3];   /* neighbor extent entries */
>>>>
>>>> case 0:
>>>>          /*
>>>>           * Filling in the middle part of a previous delayed 
>>>> allocation.
>>>>           * Contiguity is impossible here.
>>>>           * This case is avoided almost all the time.
>>>>           */
>>>> temp = new->br_startoff - PREV.br_startoff;
>>>> xfs_bmbt_set_blockcount(ep, temp);
>>>> r[0] = *new;
>>>> r[1].br_startoff = new_endoff;
>>>> temp2 = PREV.br_startoff + PREV.br_blockcount - new_endoff;
>>>> r[1].br_blockcount = temp2;
>>>> xfs_bmap_insert_exlist(ip, idx + 1, 2, &r[0], XFS_DATA_FORK);
>>>> ip->i_df.if_lastex = idx + 1;
>>>> ip->i_d.di_nextents++;
>>>>
>>>> Look at extent r[1]. It does not set br_startblock. That is, it is 
>>>> any random value. Now, look at the xfs_bmbt_set_all. Though, it sets 
>>>> the blockcount later, the startoff does not get changed.
>>>>
>>>> #if XFS_BIG_BLKNOS
>>>>         ASSERT((s->br_startblock & XFS_MASK64HI(12)) == 0);
>>>>         r->l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
>>>>                  ((xfs_bmbt_rec_base_t)s->br_startoff << 9) |
>>>>                  ((xfs_bmbt_rec_base_t)s->br_startblock >> 43);
>>>> Top 21 bits are taken as it is. However, only 9 bit should be taken. 
>>>> So, for random values, it corrupts the startoff which from 9-63 bits.
>>>
>>>
>>>  From the code inspection I agree with you that br_startblock doesn't 
>>> appear
>>> to be initialized in this scenario. Otherwise I think the code looks 
>>> good.
>>> If the br_startblock is initialized  it should be a value that fits
>>> in 52 bits out of 64 (this is what the ASSERT is for) and the top 12 
>>> bits will be 0.
>>> The r->l0 gets the top 21 bits of br_startblock, the most significant 
>>> 12 bits of
>>> which are 0 and least significant 9 could be non 0. The r->l1 gets the
>>> rest 43 (= 52-9 = 64-21) bits of br_startblock.
>>>
>>> I will open a bug report for the uninitialized br_startblock.
>>>
>>> Thank you for finding this problem.
>>>
>>> Regards,
>>> Vlad
>>>
>>>>
>>>>         r->l1 = ((xfs_bmbt_rec_base_t)s->br_startblock << 21) |
>>>>                  ((xfs_bmbt_rec_base_t)s->br_blockcount &
>>>>                  (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
>>>>
>>>> I have attached a small program which does the same thing as it is 
>>>> being done here. I would appreciate if someone can verify that 
>>>> assertion is correct.
>>>>
>>>>
>>>> Regards,
>>>> Shailendra
>>>> ------------------------------------------------------------------------ 
>>>>
>>>>
>>>> #include <stdio.h>
>>>> typedef unsigned long __uint64_t;
>>>> typedef struct xfs_bmbt_rec_64
>>>> {
>>>>         __uint64_t              l0, l1;
>>>> } xfs_bmbt_rec_64_t;
>>>>
>>>> typedef __uint64_t      xfs_bmbt_rec_base_t;    typedef 
>>>> xfs_bmbt_rec_64_t xfs_bmbt_rec_t, xfs_bmdr_rec_t;
>>>>
>>>> typedef enum {
>>>>        XFS_EXT_NORM, XFS_EXT_UNWRITTEN,
>>>>        XFS_EXT_DMAPI_OFFLINE
>>>> } xfs_exntst_t;
>>>>
>>>> typedef struct xfs_bmbt_irec
>>>> {
>>>>     __uint64_t   br_startoff;    /* starting file offset */
>>>>     __uint64_t   br_startblock;  /* starting block number */
>>>>     __uint64_t   br_blockcount;  /* number of blocks */
>>>>     xfs_exntst_t    br_state;       /* extent state */
>>>> } xfs_bmbt_irec_t;
>>>>
>>>> #define XFS_MASK64LO(n)         (((__uint64_t)1 << (n)) - 1)
>>>> #define XFS_MASK64HI(n)         ((__uint64_t)-1 << (64 - (n)))
>>>>
>>>> int main(void) {
>>>>     xfs_bmbt_irec_t s;
>>>>     xfs_bmbt_rec_t  r;
>>>>     int extent_flag;
>>>>
>>>>     s.br_startoff = 0;
>>>>     s.br_blockcount = 5;
>>>>     s.br_startblock = 0xfffffffffffffff0;
>>>>     extent_flag = (s.br_state == XFS_EXT_NORM) ? 0 : 1;
>>>>
>>>>     printf("blockcount = 0x%llx\n", s.br_startblock);
>>>>     r.l0 = ((xfs_bmbt_rec_base_t)extent_flag << 63) |
>>>>               ((xfs_bmbt_rec_base_t)s.br_startoff << 9) |
>>>>               ((xfs_bmbt_rec_base_t)s.br_startblock >> 43);
>>>>     r.l1 = ((xfs_bmbt_rec_base_t)s.br_startblock << 21) |
>>>>              ((xfs_bmbt_rec_base_t)s.br_blockcount &
>>>>              (xfs_bmbt_rec_base_t)XFS_MASK64LO(21));
>>>>
>>>>     printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
>>>>
>>>>     r.l0 = (r.l0 & (xfs_bmbt_rec_base_t)XFS_MASK64HI(55)) |
>>>>             (xfs_bmbt_rec_base_t)((__uint64_t)100 >> 43);
>>>>     r.l1 = (r.l1 & (xfs_bmbt_rec_base_t)XFS_MASK64LO(21)) |
>>>>             (xfs_bmbt_rec_base_t)((__uint64_t)100 << 21);
>>>>
>>>>     printf("l0 = 0x%llx l1 = 0x%llx\n", r.l0, r.l1);
>>>>     return 0;
>>>> }
>>>>   
>>>
>>>
>>>
>>>
> 
>