Re: Kernel 3.1.0-rc4 oops when connecting iPod

[Hin-Tak Leung <hintak_leung@yahoo.co.uk>]

--- On Tue, 6/9/11, Pavel Ivanov <paivanof@gmail.com> wrote:

> On Sat, Sep 3, 2011 at 8:37 PM,
> Hin-Tak Leung <hintak_leung@yahoo.co.uk>
> 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.

Indeed. But the differing role of the *vhdr and *buf wasn't clear.

> >> Now, the problem with "too large" error is
> trickier.
> >> According to this message
> >>
> >> >> [   92.549197] hfs: filesystem size
> too large
> >> blksz_shift=14, total_blocks=486494
> >>
> >> HFS thinks that my iPod has block size of 16 KiB.
> But
> >> generic_check_addressable() decides that
> everything with
> >> blocks larger
> >> than PAGE_CACHE_SHIFT (i.e. 4 KiB on my system)
> cannot be
> >> addressable
> >> and thus filesystem cannot be mounted. I guess it
> wasn't
> >> supposed to
> >> be that way. Is hfsplus_read_wrapper() wrong in
> determining
> >> block size
> >> or all iPods where this was tested actually had
> block size
> >> 4 KiB or
> >> less?
> >
> > Your logical sector size is 4k according to dmesg and
> hfs block size is 512 so that 16KiB is a bit dodgy.
> 
> I'm not sure where that "logical sector size" of 4k comes
> from but
> according to the sources 16K is taken directly from iPod
> via vhdr in
> hfsplus_read_wrapper(). And apparently all hfsplus code is
> designed to
> work with blocks larger than PAGE_SIZE. So it's just
> generic_check_addressable() that stands in the way. Maybe
> commit
> c6d5f5fa wasn't quite well thought through or tested by
> Christoph?

Yes, the 16k seem correct. The dmesg message is misleading.

> Anyway the following patch worked for me and I've got my
> iPod mounted
> and navigateable (although only in read-only mode because
> it has
> journaled filesystem).

I worked on the netgear journalling patches a bit:
http://htl10.users.sourceforge.net/patchsets/hfsplus_3.0_rfc/patches/
But please *back up your data*, as well as reading my notes before trying them out:
http://www.spinics.net/lists/linux-fsdevel/msg47684.html
http://www.spinics.net/lists/linux-fsdevel/msg47734.html

> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index c106ca2..5458be4 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -345,6 +345,8 @@ static int hfsplus_fill_super(struct
> super_block
> *sb, void *data, int silent)
>      struct qstr str;
>      struct nls_table *nls = NULL;
>      int err;
> +    unsigned check_blksz_bits;
> +    u64 check_num_blocks;
> 
>      err = -EINVAL;
>      sbi = kzalloc(sizeof(*sbi),
> GFP_KERNEL);
> @@ -399,10 +401,15 @@ static int hfsplus_fill_super(struct
> super_block
> *sb, void *data, int silent)
>      if (!sbi->rsrc_clump_blocks)
>         
> sbi->rsrc_clump_blocks = 1;
> 
> -    err =
> generic_check_addressable(sbi->alloc_blksz_shift,
> -           
>        
> sbi->total_blocks);
> +    check_blksz_bits =
> sbi->alloc_blksz_shift;
> +    check_num_blocks =
> sbi->total_blocks;
> +    if (sbi->fs_shift != 0) {
> +        check_num_blocks
> <<= sbi->fs_shift;
> +        check_blksz_bits -=
> sbi->fs_shift;
> +    }
> +    err =
> generic_check_addressable(check_blksz_bits,
> check_num_blocks);
>      if (err) {
>         printk(KERN_ERR "hfs:
> filesystem size too large.\n");
>          goto out_free_vhdr;
> 
> 
> I can format and submit both patches (plus a small cleanup
> that I felt
> is needed to be changed along the way). Tell me what you
> think.

This is a bit ugly - what it does is massage the two values taking into account of sbi->fs_shift to pass check_addressable() . I think either check_addressable should be extended, or this be abstracted out say, to check_hfs_addressable() (with __inline__ if desired) to be cleaner.