| Message ID | ea11fea30901200906m4dfb5060ya82104519b4984bb@mail.gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: > ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block() > where we access the b_size of it. Since it is a local variable it > might contain some garbage. Make sure it is filled with zero before > passing. Hi Ted/mingming, Any feedback on this ?? Thanks - Manish > > Signed-off-by : Manish Katiyar <mkatiyar@gmail.com> > --- > fs/ext2/super.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/ext2/super.c b/fs/ext2/super.c > index da8bdea..d10aa44 100644 > --- a/fs/ext2/super.c > +++ b/fs/ext2/super.c > @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct > super_block *sb, int type, char *data, > tocopy = sb->s_blocksize - offset < toread ? > sb->s_blocksize - offset : toread; > > - tmp_bh.b_state = 0; > + memset(&tmp_bh, 0, sizeof(struct buffer_head)); > err = ext2_get_block(inode, blk, &tmp_bh, 0); > if (err < 0) > return err; > @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct > super_block *sb, int type, > tocopy = sb->s_blocksize - offset < towrite ? > sb->s_blocksize - offset : towrite; > > - tmp_bh.b_state = 0; > + memset(&tmp_bh, 0, sizeof(struct buffer_head)); > err = ext2_get_block(inode, blk, &tmp_bh, 1); > if (err < 0) > goto out; > -- > 1.5.4.3 > > > Thanks - > Manish > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Manish Katiyar wrote: > On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: >> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block() >> where we access the b_size of it. Since it is a local variable it >> might contain some garbage. Make sure it is filled with zero before >> passing. > > Hi Ted/mingming, > > Any feedback on this ?? This looks ok to me, Manish. I'm curious, did you see this fail in real life, and if so, what'd the failure look like? With the change, the tmp_bh bh_size is 0, so maxblocks down the get_block path is also 0, but I guess that works out ok. -Eric > Thanks - > Manish > >> Signed-off-by : Manish Katiyar <mkatiyar@gmail.com> >> --- >> fs/ext2/super.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ext2/super.c b/fs/ext2/super.c >> index da8bdea..d10aa44 100644 >> --- a/fs/ext2/super.c >> +++ b/fs/ext2/super.c >> @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct >> super_block *sb, int type, char *data, >> tocopy = sb->s_blocksize - offset < toread ? >> sb->s_blocksize - offset : toread; >> >> - tmp_bh.b_state = 0; >> + memset(&tmp_bh, 0, sizeof(struct buffer_head)); >> err = ext2_get_block(inode, blk, &tmp_bh, 0); >> if (err < 0) >> return err; >> @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct >> super_block *sb, int type, >> tocopy = sb->s_blocksize - offset < towrite ? >> sb->s_blocksize - offset : towrite; >> >> - tmp_bh.b_state = 0; >> + memset(&tmp_bh, 0, sizeof(struct buffer_head)); >> err = ext2_get_block(inode, blk, &tmp_bh, 1); >> if (err < 0) >> goto out; >> -- >> 1.5.4.3 >> >> >> Thanks - >> Manish >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 25, 2009 at 9:45 PM, Eric Sandeen <sandeen@redhat.com> wrote: > Manish Katiyar wrote: >> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: >>> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block() >>> where we access the b_size of it. Since it is a local variable it >>> might contain some garbage. Make sure it is filled with zero before >>> passing. >> >> Hi Ted/mingming, >> >> Any feedback on this ?? > > This looks ok to me, Manish. I'm curious, did you see this fail in real > life, and if so, what'd the failure look like? Actually no......I realised this while going through the code. I was also wondering why we haven't hit this till now. Since ext{3,4} don't have this issue, the only reason I can think of is because ext2 with quota is not very much used or somehow we are lucky. > > With the change, the tmp_bh bh_size is 0, so maxblocks down the > get_block path is also 0, but I guess that works out ok. Yes, but that is better than having a random garbage. Isn't it ? Thanks - Manish > > -Eric > >> Thanks - >> Manish >> >>> Signed-off-by : Manish Katiyar <mkatiyar@gmail.com> >>> --- >>> fs/ext2/super.c | 4 ++-- >>> 1 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/ext2/super.c b/fs/ext2/super.c >>> index da8bdea..d10aa44 100644 >>> --- a/fs/ext2/super.c >>> +++ b/fs/ext2/super.c >>> @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct >>> super_block *sb, int type, char *data, >>> tocopy = sb->s_blocksize - offset < toread ? >>> sb->s_blocksize - offset : toread; >>> >>> - tmp_bh.b_state = 0; >>> + memset(&tmp_bh, 0, sizeof(struct buffer_head)); >>> err = ext2_get_block(inode, blk, &tmp_bh, 0); >>> if (err < 0) >>> return err; >>> @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct >>> super_block *sb, int type, >>> tocopy = sb->s_blocksize - offset < towrite ? >>> sb->s_blocksize - offset : towrite; >>> >>> - tmp_bh.b_state = 0; >>> + memset(&tmp_bh, 0, sizeof(struct buffer_head)); >>> err = ext2_get_block(inode, blk, &tmp_bh, 1); >>> if (err < 0) >>> goto out; >>> -- >>> 1.5.4.3 >>> >>> >>> Thanks - >>> Manish >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Manish Katiyar wrote: > On Sun, Jan 25, 2009 at 9:45 PM, Eric Sandeen <sandeen@redhat.com> wrote: >> Manish Katiyar wrote: >>> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: >>>> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block() >>>> where we access the b_size of it. Since it is a local variable it >>>> might contain some garbage. Make sure it is filled with zero before >>>> passing. >>> Hi Ted/mingming, >>> >>> Any feedback on this ?? >> This looks ok to me, Manish. I'm curious, did you see this fail in real >> life, and if so, what'd the failure look like? > > Actually no......I realised this while going through the code. I was > also wondering why we haven't hit this till now. Since ext{3,4} don't > have this issue, the only reason I can think of is because ext2 with > quota is not very much used or somehow we are lucky. > >> With the change, the tmp_bh bh_size is 0, so maxblocks down the >> get_block path is also 0, but I guess that works out ok. > > Yes, but that is better than having a random garbage. Isn't it ? Absolutely; it just struck me as a little odd but it's exactly what __ext2_get_block does as well, I probably just need to take a closer look at what it does w/ a 0 max. -Eric > Thanks - > Manish -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: > > ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block() > > where we access the b_size of it. Since it is a local variable it > > might contain some garbage. Make sure it is filled with zero before > > passing. > > Hi Ted/mingming, > > Any feedback on this ?? Ops, sorry. I wanted to reply but first I wanted to research more whether we can set b_size to 0 and then forgot about it. Looking into other code (e.g. in fs/mpage.c or fs/buffer.c) I think it would be better to set b_size to sb->s_blocksize and be done with that. Mapping code does not need anything else set to a deterministic value so using memset is a bit overkill. Honza > > Signed-off-by : Manish Katiyar <mkatiyar@gmail.com> > > --- > > fs/ext2/super.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/ext2/super.c b/fs/ext2/super.c > > index da8bdea..d10aa44 100644 > > --- a/fs/ext2/super.c > > +++ b/fs/ext2/super.c > > @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct > > super_block *sb, int type, char *data, > > tocopy = sb->s_blocksize - offset < toread ? > > sb->s_blocksize - offset : toread; > > > > - tmp_bh.b_state = 0; > > + memset(&tmp_bh, 0, sizeof(struct buffer_head)); > > err = ext2_get_block(inode, blk, &tmp_bh, 0); > > if (err < 0) > > return err; > > @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct > > super_block *sb, int type, > > tocopy = sb->s_blocksize - offset < towrite ? > > sb->s_blocksize - offset : towrite; > > > > - tmp_bh.b_state = 0; > > + memset(&tmp_bh, 0, sizeof(struct buffer_head)); > > err = ext2_get_block(inode, blk, &tmp_bh, 1); > > if (err < 0) > > goto out; > > -- > > 1.5.4.3 > > > > > > Thanks - > > Manish > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Sun, Jan 25, 2009 at 9:45 PM, Eric Sandeen <sandeen@redhat.com> wrote: > > Manish Katiyar wrote: > >> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: > >>> ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block() > >>> where we access the b_size of it. Since it is a local variable it > >>> might contain some garbage. Make sure it is filled with zero before > >>> passing. > >> > >> Hi Ted/mingming, > >> > >> Any feedback on this ?? > > > > This looks ok to me, Manish. I'm curious, did you see this fail in real > > life, and if so, what'd the failure look like? > > Actually no......I realised this while going through the code. I was > also wondering why we haven't hit this till now. Since ext{3,4} don't > have this issue, the only reason I can think of is because ext2 with > quota is not very much used or somehow we are lucky. Well, I think ext2 with quotas is not used very often. But also note that if a random garbage is passed in it has high chances that maxblocks is >= 1. And that is all what is needed for ext2_get_blocks() to return what we want. Honza
On Mon, Jan 26, 2009 at 5:59 PM, Jan Kara <jack@suse.cz> wrote: >> On Tue, Jan 20, 2009 at 10:36 PM, Manish Katiyar <mkatiyar@gmail.com> wrote: >> > ext2_quota_read doesn't bzeroes tmp_bh before calling ext2_get_block() >> > where we access the b_size of it. Since it is a local variable it >> > might contain some garbage. Make sure it is filled with zero before >> > passing. >> >> Hi Ted/mingming, >> >> Any feedback on this ?? > Ops, sorry. I wanted to reply but first I wanted to research more > whether we can set b_size to 0 and then forgot about it. Looking into > other code (e.g. in fs/mpage.c or fs/buffer.c) I think it would be > better to set b_size to sb->s_blocksize and be done with that. Mapping > code does not need anything else set to a deterministic value so using > memset is a bit overkill. Thanks Jan for your comments. Yes memset is an overkill. I did it just because other users of ext2_get_block were doing the same way. Will rework the patch with setting b_size as blocksize and send again. Thanks - Manish > > Honza > >> > Signed-off-by : Manish Katiyar <mkatiyar@gmail.com> >> > --- >> > fs/ext2/super.c | 4 ++-- >> > 1 files changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/fs/ext2/super.c b/fs/ext2/super.c >> > index da8bdea..d10aa44 100644 >> > --- a/fs/ext2/super.c >> > +++ b/fs/ext2/super.c >> > @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct >> > super_block *sb, int type, char *data, >> > tocopy = sb->s_blocksize - offset < toread ? >> > sb->s_blocksize - offset : toread; >> > >> > - tmp_bh.b_state = 0; >> > + memset(&tmp_bh, 0, sizeof(struct buffer_head)); >> > err = ext2_get_block(inode, blk, &tmp_bh, 0); >> > if (err < 0) >> > return err; >> > @@ -1366,7 +1366,7 @@ static ssize_t ext2_quota_write(struct >> > super_block *sb, int type, >> > tocopy = sb->s_blocksize - offset < towrite ? >> > sb->s_blocksize - offset : towrite; >> > >> > - tmp_bh.b_state = 0; >> > + memset(&tmp_bh, 0, sizeof(struct buffer_head)); >> > err = ext2_get_block(inode, blk, &tmp_bh, 1); >> > if (err < 0) >> > goto out; >> > -- >> > 1.5.4.3 >> > >> > >> > Thanks - >> > Manish >> > >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > Jan Kara <jack@suse.cz> > SuSE CR Labs > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext2/super.c b/fs/ext2/super.c index da8bdea..d10aa44 100644 --- a/fs/ext2/super.c +++ b/fs/ext2/super.c @@ -1327,7 +1327,7 @@ static ssize_t ext2_quota_read(struct super_block *sb, int type, char *data, tocopy = sb->s_blocksize - offset < toread ? sb->s_blocksize - offset : toread; - tmp_bh.b_state = 0; + memset(&tmp_bh, 0, sizeof(struct buffer_head)); err = ext2_get_block(inode, blk, &tmp_bh, 0); if (err < 0)