From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: with ECARTIS (v1.0.0; list xfs); Tue, 14 Nov 2006 16:45:06 -0800 (PST) Received: from cuda.sgi.com (cuda1.sgi.com [192.48.168.28]) by oss.sgi.com (8.12.10/8.12.10/SuSE Linux 0.7) with ESMTP id kAF0ivaG018540 for ; Tue, 14 Nov 2006 16:44:59 -0800 Message-ID: <455A600D.8010803@agami.com> Date: Tue, 14 Nov 2006 16:32:13 -0800 From: Shailendra Tripathi MIME-Version: 1.0 Subject: Re: xfs_bmap_add_extent_delay_real: Uninited r[3] corrupts startoff References: <4529F8A8.6080900@agami.com> <452C44A2.7000907@sgi.com> <455A589E.4040607@sgi.com> In-Reply-To: <455A589E.4040607@sgi.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: xfs-bounce@oss.sgi.com Errors-to: xfs-bounce@oss.sgi.com List-Id: xfs To: lachlan@sgi.com Cc: Vlad Apostolov , xfs mailing list , xfs-dev@sgi.com 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 >>> 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; >>> } >>> >> >> >>