Re: Kernel 3.1.0-rc4 oops when connecting iPod

[Seth Forshee]

On Tue, Sep 06, 2011 at 12:35:22AM -0400, Pavel Ivanov wrote:
> On Sat, Sep 3, 2011 at 8:37 PM, Hin-Tak Leung <hintak_leung@xxxxxxxxxxx> wrote:
> >> I've looked into the code myself a little and here's what I
> >> see.
> >> hfsplus_read_wrapper() calls to hfsplus_submit_bio() twice
> >> to fill
> >> sbi->s_vhdr and sbi->s_backup_vhdr. And according to
> >> parameters they
> >> are filled with pointers into sbi->s_vhdr_buf and
> >> sbi->s_backup_vhdr_buf respectively. It's done with the
> >> following code
> >> at fs/hfsplus/wrapper.c:79:
> >>
> >> ÂÂÂ if (!(rw & WRITE) && data)
> >> ÂÂÂ ÂÂÂ *data = (u8 *)buf +
> >> offset;
> >>
> >> So s_vhdr and s_backup_vhdr shouldn't be freed, s_vhdr_buf
> >> and
> >> s_backup_vhdr_buf should be freed instead. And indeed
> >> changing in
> >> hfsplus_fill_super()
> >>
> >> ÂÂÂ kfree(sbi->s_vhdr);
> >> ÂÂÂ kfree(sbi->s_backup_vhdr);
> >>
> >> to
> >>
> >> ÂÂÂ kfree(sbi->s_vhdr_buf);
> >> ÂÂÂ kfree(sbi->s_backup_vhdr_buf);
> >>
> >> fixes BUG reports from SLUB.
> >
> > The code around there is a bit too dense, and both of the *_buf are recent introductions (and temp values, I think) as is hfsplus_submit_bio() itself, around the 2.6.39/3.0 time frame. I think the intention is to fill s_vhdr/s_backup_vhdr via mulitple fetches using *_buf as temp buffer.
> 
> Well, look at the commit 6596528e. It clearly shows that result of
> kmalloc() is no longer assigned to sbi->s_vhdr and sbi->s_backup_vhdr,
> but is assigned to sbi->s_vhdr_buf and sbi->s_backup_vhdr_buf. Also
> this commit clearly changes hfsplus_put_super() so that it doesn't
> free sbi->s_vhdr and sbi->s_backup_vhdr, but frees sbi->s_vhdr_buf and
> sbi->s_backup_vhdr_buf instead. I guess Seth just missed
> hfsplus_fill_super() in there as it's pretty unusual exit path.

Yes, that was definitely just an oversight. Has anyone provided a patch
yet? If not I've pasted a patch below. Seems like a fix should be
applied ASAP.