Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

Kernel 3.1.0-rc4 oops when connecting iPod

 

 

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded


paivanof at gmail

Sep 2, 2011, 8:08 PM

Post #1 of 29 (1377 views)
Permalink
Kernel 3.1.0-rc4 oops when connecting iPod

Hi,

With kernel 3.1.0-rc4 any attempt to connect iPod to USB leads to
kernel oops. I'd say that stacktrace of the oops is pretty much random
and not related to HFS. But I was able to get useful info from it when
I recompiled with CONFIG_SLUB_DEBUG_ON=y. In this case I don't get
oops but the following instead:

[ 81.508807] hfs: filesystem size too large.
[ 81.559295] =============================================================================
[ 81.656965] BUG kmalloc-4096: Invalid object pointer 0xffff880136160400
[ 81.735947] -----------------------------------------------------------------------------
[ 81.735948]
[ 81.851359] INFO: Slab 0xffffea0004d85800 objects=7 used=7 fp=0x
(null) flags=0x2000000000004081
[ 81.965628] Pid: 2086, comm: ipod-set-info Not tainted 3.1.0-rc4+ #2
[ 81.965629] Call Trace:
[ 81.965636] [<ffffffff8119359f>] slab_err+0xaf/0xd0
[ 81.965640] [<ffffffff8119381f>] ? slab_pad_check+0xaf/0x1a0
[ 81.965644] [<ffffffff8119719f>] free_debug_processing+0x1ef/0x310
[ 81.965647] [<ffffffff811974ba>] __slab_free+0x1fa/0x490
[ 81.965653] [<ffffffffa0516c4d>] ? hfsplus_fill_super+0x2ed/0x610 [hfsplus]
[ 81.965656] [<ffffffff81197a3c>] kfree+0x12c/0x150
[ 81.965660] [<ffffffffa0516c4d>] hfsplus_fill_super+0x2ed/0x610 [hfsplus]
[ 81.965665] [<ffffffff81438771>] ? ioctl_internal_command.clone.4+0x61/0x1b0
[ 81.965671] [<ffffffff8161fe9e>] ? _raw_spin_unlock+0xe/0x40
[ 81.965675] [<ffffffff81195eb8>] ? deactivate_slab+0x578/0x750
[ 81.965679] [<ffffffff811af3cb>] ? sget+0xab/0x480
[ 81.965683] [<ffffffff8133f0c9>] ? put_dec+0x59/0x60
[ 81.965685] [<ffffffff813407d1>] ? number.clone.2+0x311/0x350
[ 81.965690] [<ffffffff8116bd7b>] ? pcpu_alloc_area+0x10b/0x2c0
[ 81.965693] [<ffffffff81341b81>] ? vsnprintf+0x471/0x610
[ 81.965696] [<ffffffff8116c639>] ? pcpu_alloc+0x399/0x9e0
[ 81.965699] [<ffffffff81341dc4>] ? snprintf+0x34/0x40
[ 81.965702] [<ffffffff8133ebc6>] ? strlcpy+0x46/0x60
[ 81.965707] [<ffffffff8115bfb2>] ? register_shrinker+0x52/0x60
[ 81.965710] [<ffffffff811afa26>] mount_bdev+0x1c6/0x210
[ 81.965714] [<ffffffffa0516960>] ? hfsplus_iget+0x240/0x240 [hfsplus]
[ 81.965718] [<ffffffffa05160a5>] hfsplus_mount+0x15/0x20 [hfsplus]
[ 81.965720] [<ffffffff811b0547>] mount_fs+0x47/0x1c0
[ 81.965723] [<ffffffff8116cc90>] ? __alloc_percpu+0x10/0x20
[ 81.965727] [<ffffffff811ca383>] vfs_kern_mount+0x63/0xd0
[ 81.965731] [<ffffffff811cb4d4>] do_kern_mount+0x54/0x110
[ 81.965734] [<ffffffff811cd022>] do_mount+0x502/0x7e0
[ 81.965736] [<ffffffff8116738b>] ? strndup_user+0x5b/0x80
[ 81.965739] [<ffffffff811cd710>] sys_mount+0x90/0xe0
[ 81.965744] [<ffffffff81627402>] system_call_fastpath+0x16/0x1b
[ 81.965746] FIX kmalloc-4096: Object at 0xffff880136160400 not freed

Following by 2 other bugs with the same stack trace.

(BTW, before oops first message "hfs: filesystem size too large"
always appears twice.)


Another question: is hfsplus supposed to work at all? I see it's
marked as "Orphaned" in MAINTAINERS and on attempt to connect iPod
with kernel 2.6.38.11 (standard kernel in Ubuntu 11.04) I still don't
get it working. Although without oops I get the following messages:

[ 65.479360] usb 2-3: new high speed USB device using ehci_hcd and address 4
[ 65.686470] usbcore: registered new interface driver uas
[ 65.706455] Initializing USB Mass Storage driver...
[ 65.706901] scsi4 : usb-storage 2-3:1.0
[ 65.707251] usbcore: registered new interface driver usb-storage
[ 65.707254] USB Mass Storage support registered.
[ 66.702320] scsi 4:0:0:0: Direct-Access Apple iPod
1.62 PQ: 0 ANSI: 0
[ 66.704537] sd 4:0:0:0: Attached scsi generic sg2 type 0
[ 66.809047] sd 4:0:0:0: [sdb] 1946049 4096-byte logical blocks:
(7.97 GB/7.42 GiB)
[ 66.809930] sd 4:0:0:0: [sdb] Write Protect is off
[ 66.809935] sd 4:0:0:0: [sdb] Mode Sense: 68 00 00 08
[ 66.809938] sd 4:0:0:0: [sdb] Assuming drive cache: write through
[ 66.885037] sd 4:0:0:0: [sdb] 1946049 4096-byte logical blocks:
(7.97 GB/7.42 GiB)
[ 66.885933] sd 4:0:0:0: [sdb] Assuming drive cache: write through
[ 66.979400] sdb: [mac] sdb1 sdb2
[ 66.981940] sd 4:0:0:0: [sdb] 1946049 4096-byte logical blocks:
(7.97 GB/7.42 GiB)
[ 66.982834] sd 4:0:0:0: [sdb] Assuming drive cache: write through
[ 67.055808] sd 4:0:0:0: [sdb] Attached SCSI removable disk
[ 67.299357] sd 4:0:0:0: [sdb] Bad block number requested
[ 67.362982] hfs: unable to find HFS+ superblock
[ 67.499457] sd 4:0:0:0: [sdb] Bad block number requested
[ 67.563085] hfs: unable to find HFS+ superblock


Thank you,
Pavel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hintak_leung at yahoo

Sep 2, 2011, 8:59 PM

Post #2 of 29 (1372 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

--- On Sat, 3/9/11, Pavel Ivanov <paivanof [at] gmail> wrote:

> Hi,
>
> With kernel 3.1.0-rc4 any attempt to connect iPod to USB
> leads to
> kernel oops. I'd say that stacktrace of the oops is pretty
> much random
> and not related to HFS. But I was able to get useful info
> from it when
> I recompiled with CONFIG_SLUB_DEBUG_ON=y. In this case I
> don't get
> oops but the following instead:

There are a few hfsplus related changes to do protection against invalid data like this, but may be there are more. It would be useful to have the output from your
objdump -l -d hfsplus.ko | grep -A 1000 '<hfsplus_fill_super>'
(the -l gives line numbers against the kernel tree, so would be useful if you run this against the ko there...)

>
> [81.508807] hfs: filesystem size too
> large.
> [81.559295]
> =============================================================================
> [81.656965] BUG kmalloc-4096: Invalid
> object pointer 0xffff880136160400
> [81.735947]
> -----------------------------------------------------------------------------
> [81.735948]
> [81.851359] INFO: Slab 0xffffea0004d85800
> objects=7 used=7 fp=0x
> (null) flags=0x2000000000004081
> [81.965628] Pid: 2086, comm:
> ipod-set-info Not tainted 3.1.0-rc4+ #2
> [81.965629] Call Trace:
> [81.965636]
> [<ffffffff8119359f>] slab_err+0xaf/0xd0
> [81.965640]
> [<ffffffff8119381f>] ? slab_pad_check+0xaf/0x1a0
> [81.965644]
> [<ffffffff8119719f>]
> free_debug_processing+0x1ef/0x310
> [81.965647]
> [<ffffffff811974ba>] __slab_free+0x1fa/0x490
> [81.965653]
> [<ffffffffa0516c4d>] ? hfsplus_fill_super+0x2ed/0x610
> [hfsplus]
> [81.965656]
> [<ffffffff81197a3c>] kfree+0x12c/0x150
> [81.965660]
> [<ffffffffa0516c4d>] hfsplus_fill_super+0x2ed/0x610
> [hfsplus]
> [81.965665]
> [<ffffffff81438771>] ?
> ioctl_internal_command.clone.4+0x61/0x1b0
> [81.965671]
> [<ffffffff8161fe9e>] ? _raw_spin_unlock+0xe/0x40
> [81.965675]
> [<ffffffff81195eb8>] ? deactivate_slab+0x578/0x750
> [81.965679]
> [<ffffffff811af3cb>] ? sget+0xab/0x480
> [81.965683]
> [<ffffffff8133f0c9>] ? put_dec+0x59/0x60
> [81.965685]
> [<ffffffff813407d1>] ? number.clone.2+0x311/0x350
> [81.965690]
> [<ffffffff8116bd7b>] ? pcpu_alloc_area+0x10b/0x2c0
> [81.965693]
> [<ffffffff81341b81>] ? vsnprintf+0x471/0x610
> [81.965696]
> [<ffffffff8116c639>] ? pcpu_alloc+0x399/0x9e0
> [81.965699]
> [<ffffffff81341dc4>] ? snprintf+0x34/0x40
> [81.965702]
> [<ffffffff8133ebc6>] ? strlcpy+0x46/0x60
> [81.965707]
> [<ffffffff8115bfb2>] ? register_shrinker+0x52/0x60
> [81.965710]
> [<ffffffff811afa26>] mount_bdev+0x1c6/0x210
> [81.965714]
> [<ffffffffa0516960>] ? hfsplus_iget+0x240/0x240
> [hfsplus]
> [81.965718]
> [<ffffffffa05160a5>] hfsplus_mount+0x15/0x20
> [hfsplus]
> [81.965720]
> [<ffffffff811b0547>] mount_fs+0x47/0x1c0
> [81.965723]
> [<ffffffff8116cc90>] ? __alloc_percpu+0x10/0x20
> [81.965727]
> [<ffffffff811ca383>] vfs_kern_mount+0x63/0xd0
> [81.965731]
> [<ffffffff811cb4d4>] do_kern_mount+0x54/0x110
> [81.965734]
> [<ffffffff811cd022>] do_mount+0x502/0x7e0
> [81.965736]
> [<ffffffff8116738b>] ? strndup_user+0x5b/0x80
> [81.965739]
> [<ffffffff811cd710>] sys_mount+0x90/0xe0
> [81.965744]
> [<ffffffff81627402>] system_call_fastpath+0x16/0x1b
> [81.965746] FIX kmalloc-4096: Object at
> 0xffff880136160400 not freed
>
> Following by 2 other bugs with the same stack trace.
>
> (BTW, before oops first message "hfs: filesystem size too
> large"
> always appears twice.)
>
>
> Another question: is hfsplus supposed to work at all? I see
> it's
> marked as "Orphaned" in MAINTAINERS and on attempt to
> connect iPod
> with kernel 2.6.38.11 (standard kernel in Ubuntu 11.04) I
> still don't
> get it working. Although without oops I get the following
> messages:
>
> [65.479360] usb 2-3: new high speed USB
> device using ehci_hcd and address 4
> [65.686470] usbcore: registered new
> interface driver uas
> [65.706455] Initializing USB Mass Storage
> driver...
> [65.706901] scsi4 : usb-storage 2-3:1.0
> [65.707251] usbcore: registered new
> interface driver usb-storage
> [65.707254] USB Mass Storage support
> registered.
> [66.702320] scsi 4:0:0:0:
> Direct-Access Apple
> iPod
> 1.62 PQ: 0 ANSI: 0
> [66.704537] sd 4:0:0:0: Attached scsi
> generic sg2 type 0
> [66.809047] sd 4:0:0:0: [sdb] 1946049
> 4096-byte logical blocks:
> (7.97 GB/7.42 GiB)
> [66.809930] sd 4:0:0:0: [sdb] Write
> Protect is off
> [66.809935] sd 4:0:0:0: [sdb] Mode Sense:
> 68 00 00 08
> [66.809938] sd 4:0:0:0: [sdb] Assuming
> drive cache: write through
> [66.885037] sd 4:0:0:0: [sdb] 1946049
> 4096-byte logical blocks:
> (7.97 GB/7.42 GiB)
> [66.885933] sd 4:0:0:0: [sdb] Assuming
> drive cache: write through
> [66.979400] sdb: [mac] sdb1 sdb2
> [66.981940] sd 4:0:0:0: [sdb] 1946049
> 4096-byte logical blocks:
> (7.97 GB/7.42 GiB)
> [66.982834] sd 4:0:0:0: [sdb] Assuming
> drive cache: write through
> [67.055808] sd 4:0:0:0: [sdb] Attached
> SCSI removable disk
> [67.299357] sd 4:0:0:0: [sdb] Bad block
> number requested
> [67.362982] hfs: unable to find HFS+
> superblock
> [67.499457] sd 4:0:0:0: [sdb] Bad block
> number requested
> [67.563085] hfs: unable to find HFS+
> superblock
>
>
> Thank you,
> Pavel
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-fsdevel" in
> the body of a message to majordomo [at] vger
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 2, 2011, 9:37 PM

Post #3 of 29 (1361 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Fri, Sep 2, 2011 at 11:59 PM, Hin-Tak Leung <hintak_leung [at] yahoo> wrote:
>> With kernel 3.1.0-rc4 any attempt to connect iPod to USB
>> leads to
>> kernel oops. I'd say that stacktrace of the oops is pretty
>> much random
>> and not related to HFS. But I was able to get useful info
>> from it when
>> I recompiled with CONFIG_SLUB_DEBUG_ON=y. In this case I
>> don't get
>> oops but the following instead:
>
> There are a few hfsplus related changes to do protection against invalid data like this, but may be there are more. It would be useful to have the output from your
> objdump -l -d hfsplus.ko | grep -A 1000 '<hfsplus_fill_super>'
> (the -l gives line numbers against the kernel tree, so would be useful if you run this against the ko there...)

Output of this command is in attachment.


Pavel
Attachments: hfsplus.dump (52.2 KB)


hintak_leung at yahoo

Sep 2, 2011, 11:57 PM

Post #4 of 29 (1366 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

--- On Sat, 3/9/11, Pavel Ivanov <paivanof [at] gmail> wrote:

> On Fri, Sep 2, 2011 at 11:59 PM,
> Hin-Tak Leung <hintak_leung [at] yahoo>
> wrote:
> >> With kernel 3.1.0-rc4 any attempt to connect iPod
> to USB
> >> leads to
> >> kernel oops. I'd say that stacktrace of the oops
> is pretty
> >> much random
> >> and not related to HFS. But I was able to get
> useful info
> >> from it when
> >> I recompiled with CONFIG_SLUB_DEBUG_ON=y. In this
> case I
> >> don't get
> >> oops but the following instead:
> >
> > There are a few hfsplus related changes to do
> protection against invalid data like this, but may be there
> are more. It would be useful to have the output from your
> > objdump -l -d hfsplus.ko | grep -A 1000
> '<hfsplus_fill_super>'
> > (the -l gives line numbers against the kernel tree, so
> would be useful if you run this against the ko there...)
>
> Output of this command is in attachment.

That's interesting. You said "hfs: filesystem size too large." always appears twice (with kernel 3.1-rc4) before it oops. And in your 2.6.38.11 kernel, you had "hfs: unable to find HFS+ superblock" twice.

The oops place is the "kfree(sbi->s_backup_vhdr)" in line 529 in fs/hfsplus/super.c:

527: out_free_vhdr:
528: kfree(sbi->s_vhdr);
529: kfree(sbi->s_backup_vhdr);

It would appear the s_backup_vhdr is somehow garbage but that was not caught in the 3.1-rc4 version of hfsplus_read_wrapper() ; it was caught by the 2.6.38.11 version of hfsplus_read_wrapper(). hfsplus_read_wrapper() was changed in the 2.6.39/3.0 time frame by this:

commit 52399b171dfaea02b6944cd6feba49b624147126
Author: Christoph Hellwig <hch [at] tuxera>
Date: Tue Nov 23 14:37:47 2010 +0100

hfsplus: use raw bio access for the volume headers

That's code I don't quite understand (I worked on the hfsplus journal code recently, supposedly mentoring for that GSoC project).

If you are happy enough to do a bit of experimenting, can you try putting a
"if(sbi->s_backup_vhdr)" before line 529?

Also it is curious why it wasn't caught in wrapper.c arond 229 to 236 ending with:
"if (sbi->s_backup_vhdr->signature != sbi->s_vhdr->signature)"


The file system too large comes from line 402 in super.c:
-----------------------
err = generic_check_addressable(sbi->alloc_blksz_shift,
sbi->total_blocks);
if (err) {
printk(KERN_ERR "hfs: filesystem size too large.\n");
goto out_free_vhdr;

-----------------------
So it might be interesting to see what is too large... try changing that to:

printk(KERN_ERR "hfs: filesystem size too large blksz_shift=%d, total_blocks=%d\n", sbi->alloc_blksz_shift, sbi->total_blocks);

?

It is a 42GB image - if it were smaller I would suggest dd'ing that and upload it somewhere to check...



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 3, 2011, 1:52 PM

Post #5 of 29 (1366 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

Hin-Tak,

"if(sbi->s_backup_vhdr)" as you suggested didn't help. But I made
another change. I made fs/hfsplus/super.c to look like this near the
line 529:

out_free_vhdr:
printk(KERN_ERR "hfs: sbi->s_vhdr = %p, sbi->s_backup_vhdr = %p\n",
sbi->s_vhdr, sbi->s_backup_vhdr);
kfree(sbi->s_vhdr);
kfree(sbi->s_backup_vhdr);

And here's what I see after connecting an iPod:

[ 92.549197] hfs: filesystem size too large blksz_shift=14,
total_blocks=486494
[ 92.635714] hfs: sbi->s_vhdr = ffff88013703a690, sbi->s_backup_vhdr
= ffff88013703e268
[ 92.730543] =============================================================================
[ 92.828425] BUG kmalloc-4096: Invalid object pointer 0xffff88013703a690
...
[ 93.213890] =============================================================================
[ 93.311834] BUG kmalloc-4096: Invalid object pointer 0xffff88013703e268
...
[ 93.868618] hfs: filesystem size too large blksz_shift=14,
total_blocks=486494
[ 93.955327] hfs: sbi->s_vhdr = ffff8801343c37d8, sbi->s_backup_vhdr
= ffff8801343c5120
[ 94.050133] =============================================================================
[ 94.148026] BUG kmalloc-4096: Invalid object pointer 0xffff8801343c37d8
...
[ 94.533895] =============================================================================
[ 94.631746] BUG kmalloc-4096: Invalid object pointer 0xffff8801343c5120


So these 2 pointers are exactly what causing the trouble.


Pavel


On Sat, Sep 3, 2011 at 2:57 AM, Hin-Tak Leung <hintak_leung [at] yahoo> wrote:
> --- On Sat, 3/9/11, Pavel Ivanov <paivanof [at] gmail> wrote:
>
>> On Fri, Sep 2, 2011 at 11:59 PM,
>> Hin-Tak Leung <hintak_leung [at] yahoo>
>> wrote:
>> >> With kernel 3.1.0-rc4 any attempt to connect iPod
>> to USB
>> >> leads to
>> >> kernel oops. I'd say that stacktrace of the oops
>> is pretty
>> >> much random
>> >> and not related to HFS. But I was able to get
>> useful info
>> >> from it when
>> >> I recompiled with CONFIG_SLUB_DEBUG_ON=y. In this
>> case I
>> >> don't get
>> >> oops but the following instead:
>> >
>> > There are a few hfsplus related changes to do
>> protection against invalid data like this, but may be there
>> are more. It would be useful to have the output from your
>> > objdump -l -d hfsplus.ko | grep -A 1000
>> '<hfsplus_fill_super>'
>> > (the -l gives line numbers against the kernel tree, so
>> would be useful if you run this against the ko there...)
>>
>> Output of this command is in attachment.
>
> That's interesting. You said "hfs: filesystem size too large." always appears twice (with kernel 3.1-rc4) before it oops. And in your 2.6.38.11 kernel, you had "hfs: unable to find HFS+ superblock" twice.
>
> The oops place is the "kfree(sbi->s_backup_vhdr)" in line 529 in fs/hfsplus/super.c:
>
> 527: out_free_vhdr:
> 528: kfree(sbi->s_vhdr);
> 529: kfree(sbi->s_backup_vhdr);
>
> It would appear the s_backup_vhdr is somehow garbage but that was not caught in the 3.1-rc4 version of hfsplus_read_wrapper() ; it was caught by the 2.6.38.11 version of hfsplus_read_wrapper(). hfsplus_read_wrapper() was changed in the 2.6.39/3.0 time frame by this:
>
> commit 52399b171dfaea02b6944cd6feba49b624147126
> Author: Christoph Hellwig <hch [at] tuxera>
> Date: Tue Nov 23 14:37:47 2010 +0100
>
> hfsplus: use raw bio access for the volume headers
>
> That's code I don't quite understand (I worked on the hfsplus journal code recently, supposedly mentoring for that GSoC project).
>
> If you are happy enough to do a bit of experimenting, can you try putting a
> "if(sbi->s_backup_vhdr)" before line 529?
>
> Also it is curious why it wasn't caught in wrapper.c arond 229 to 236 ending with:
> "if (sbi->s_backup_vhdr->signature != sbi->s_vhdr->signature)"
>
>
> The file system too large comes from line 402 in super.c:
> -----------------------
> err = generic_check_addressable(sbi->alloc_blksz_shift,
> sbi->total_blocks);
> if (err) {
> printk(KERN_ERR "hfs: filesystem size too large.\n");
> goto out_free_vhdr;
>
> -----------------------
> So it might be interesting to see what is too large... try changing that to:
>
> printk(KERN_ERR "hfs: filesystem size too large blksz_shift=%d, total_blocks=%d\n", sbi->alloc_blksz_shift, sbi->total_blocks);
>
> ?
>
> It is a 42GB image - if it were smaller I would suggest dd'ing that and upload it somewhere to check...
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hintak_leung at yahoo

Sep 3, 2011, 4:35 PM

Post #6 of 29 (1360 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

--- On Sat, 3/9/11, Pavel Ivanov <paivanof [at] gmail> wrote:

> Hin-Tak,
>
> "if(sbi->s_backup_vhdr)" as you suggested didn't help.
> But I made
> another change. I made fs/hfsplus/super.c to look like this
> near the
> line 529:
>
> out_free_vhdr:
> printk(KERN_ERR "hfs: sbi->s_vhdr =
> %p, sbi->s_backup_vhdr = %p\n",
> sbi->s_vhdr, sbi->s_backup_vhdr);
> kfree(sbi->s_vhdr);
> kfree(sbi->s_backup_vhdr);
>
> And here's what I see after connecting an iPod:
>
> [92.549197] hfs: filesystem size too
> large blksz_shift=14,
> total_blocks=486494
> [92.635714] hfs: sbi->s_vhdr =
> ffff88013703a690, sbi->s_backup_vhdr
> = ffff88013703e268
> [92.730543]
> =============================================================================
> [92.828425] BUG kmalloc-4096: Invalid
> object pointer 0xffff88013703a690
> ...
> [93.213890]
> =============================================================================
> [93.311834] BUG kmalloc-4096: Invalid
> object pointer 0xffff88013703e268
> ...
> [93.868618] hfs: filesystem size too
> large blksz_shift=14,
> total_blocks=486494
> [93.955327] hfs: sbi->s_vhdr =
> ffff8801343c37d8, sbi->s_backup_vhdr
> = ffff8801343c5120
> [94.050133]
> =============================================================================
> [94.148026] BUG kmalloc-4096: Invalid
> object pointer 0xffff8801343c37d8
> ...
> [94.533895]
> =============================================================================
> [94.631746] BUG kmalloc-4096: Invalid
> object pointer 0xffff8801343c5120
>
>
> So these 2 pointers are exactly what causing the trouble.

This is interesting... so it would appear that hfsplus_read_wrapper() isn't working correctly, but enough of correct information to pass checks. I just re-read your e-mail and your device has a logical block size of 4096 bytes, whereas most of the hfsplus code uses a block size of 512... you will need to look into hfsplus_submit_bio(), which is in the same file wrapper.c.

It is going to be difficult to do anything without the actual device and 8GB is too large to be send around. Assuming it is mostly music/media and there isn't too much stuff which is too confident/private, can I ask you to send me say, the first few MB of the disk? I guess something like this:

dd if=/dev/sdb of=diskfile-sdb ibs=4096 count=512
dd if=/dev/sdb1 of=diskfile-sdb1 ibs=4096 count=512
dd if=/dev/sdb2 of=diskfile-sdb2 ibs=4096 count=512

should copy the first 2MB of the disk itself and the two partitions. I hope that's enough to have a look around. Be really careful with dd though - it could do some serious damage if not carefully used.

> On Sat, Sep 3, 2011 at 2:57 AM, Hin-Tak Leung <hintak_leung [at] yahoo>
> wrote:
> > --- On Sat, 3/9/11, Pavel Ivanov <paivanof [at] gmail>
> wrote:
> >
> >> On Fri, Sep 2, 2011 at 11:59 PM,
> >> Hin-Tak Leung <hintak_leung [at] yahoo>
> >> wrote:
> >> >> With kernel 3.1.0-rc4 any attempt to
> connect iPod
> >> to USB
> >> >> leads to
> >> >> kernel oops. I'd say that stacktrace of
> the oops
> >> is pretty
> >> >> much random
> >> >> and not related to HFS. But I was able to
> get
> >> useful info
> >> >> from it when
> >> >> I recompiled with CONFIG_SLUB_DEBUG_ON=y.
> In this
> >> case I
> >> >> don't get
> >> >> oops but the following instead:
> >> >
> >> > There are a few hfsplus related changes to
> do
> >> protection against invalid data like this, but may
> be there
> >> are more. It would be useful to have the output
> from your
> >> > objdump -l -d hfsplus.ko | grep -A 1000
> >> '<hfsplus_fill_super>'
> >> > (the -l gives line numbers against the kernel
> tree, so
> >> would be useful if you run this against the ko
> there...)
> >>
> >> Output of this command is in attachment.
> >
> > That's interesting. You said "hfs: filesystem size too
> large." always appears twice (with kernel 3.1-rc4) before it
> oops. And in your 2.6.38.11 kernel, you had "hfs: unable to
> find HFS+ superblock" twice.
> >
> > The oops place is the "kfree(sbi->s_backup_vhdr)"
> in line 529 in fs/hfsplus/super.c:
> >
> > 527: out_free_vhdr:
> > 528: kfree(sbi->s_vhdr);
> > 529: kfree(sbi->s_backup_vhdr);
> >
> > It would appear the s_backup_vhdr is somehow garbage
> but that was not caught in the 3.1-rc4 version of
> hfsplus_read_wrapper() ; it was caught by the 2.6.38.11
> version of hfsplus_read_wrapper(). hfsplus_read_wrapper()
> was changed in the 2.6.39/3.0 time frame by this:
> >
> > commit 52399b171dfaea02b6944cd6feba49b624147126
> > Author: Christoph Hellwig <hch [at] tuxera>
> > Date: Tue Nov 23 14:37:47 2010 +0100
> >
> > hfsplus: use raw bio access for the volume
> headers
> >
> > That's code I don't quite understand (I worked on the
> hfsplus journal code recently, supposedly mentoring for that
> GSoC project).
> >
> > If you are happy enough to do a bit of experimenting,
> can you try putting a
> > "if(sbi->s_backup_vhdr)" before line 529?
> >
> > Also it is curious why it wasn't caught in wrapper.c
> arond 229 to 236 ending with:
> > "if (sbi->s_backup_vhdr->signature !=
> sbi->s_vhdr->signature)"
> >
> >
> > The file system too large comes from line 402 in
> super.c:
> > -----------------------
> > err =
> generic_check_addressable(sbi->alloc_blksz_shift,
> >
> sbi->total_blocks);
> > if (err) {
> > printk(KERN_ERR "hfs:
> filesystem size too large.\n");
> > goto out_free_vhdr;
> >
> > -----------------------
> > So it might be interesting to see what is too large...
> try changing that to:
> >
> > printk(KERN_ERR "hfs: filesystem size too large
> blksz_shift=%d, total_blocks=%d\n",
> sbi->alloc_blksz_shift, sbi->total_blocks);
> >
> > ?
> >
> > It is a 42GB image - if it were smaller I would
> suggest dd'ing that and upload it somewhere to check...
> >
> >
> >
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 3, 2011, 4:59 PM

Post #7 of 29 (1343 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Sat, Sep 3, 2011 at 7:35 PM, Hin-Tak Leung <hintak_leung [at] yahoo> wrote:
>> "if(sbi->s_backup_vhdr)" as you suggested didn't help.
>> But I made
>> another change. I made fs/hfsplus/super.c to look like this
>> near the
>> line 529:
>>
>> out_free_vhdr:
>> printk(KERN_ERR "hfs: sbi->s_vhdr =
>> %p, sbi->s_backup_vhdr = %p\n",
>> sbi->s_vhdr, sbi->s_backup_vhdr);
>> kfree(sbi->s_vhdr);
>> kfree(sbi->s_backup_vhdr);
>>
>> And here's what I see after connecting an iPod:
>>
>> [92.549197] hfs: filesystem size too
>> large blksz_shift=14,
>> total_blocks=486494
>> [92.635714] hfs: sbi->s_vhdr =
>> ffff88013703a690, sbi->s_backup_vhdr
>> = ffff88013703e268
>> [92.730543]
>> =============================================================================
>> [92.828425] BUG kmalloc-4096: Invalid
>> object pointer 0xffff88013703a690
>> ...
>> [93.213890]
>> =============================================================================
>> [93.311834] BUG kmalloc-4096: Invalid
>> object pointer 0xffff88013703e268
>> ...
>> [93.868618] hfs: filesystem size too
>> large blksz_shift=14,
>> total_blocks=486494
>> [93.955327] hfs: sbi->s_vhdr =
>> ffff8801343c37d8, sbi->s_backup_vhdr
>> = ffff8801343c5120
>> [94.050133]
>> =============================================================================
>> [94.148026] BUG kmalloc-4096: Invalid
>> object pointer 0xffff8801343c37d8
>> ...
>> [94.533895]
>> =============================================================================
>> [94.631746] BUG kmalloc-4096: Invalid
>> object pointer 0xffff8801343c5120
>>
>>
>> So these 2 pointers are exactly what causing the trouble.
>
> This is interesting... so it would appear that hfsplus_read_wrapper() isn't working correctly, but enough of correct information to pass checks. I just re-read your e-mail and your device has a logical block size of 4096 bytes, whereas most of the hfsplus code uses a block size of 512... you will need to look into hfsplus_submit_bio(), which is in the same file wrapper.c.

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.

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?

> It is going to be difficult to do anything without the actual device and 8GB is too large to be send around. Assuming it is mostly music/media and there isn't too much stuff which is too confident/private, can I ask you to send me say, the first few MB of the disk?

I'll send it in a separate email bypassing lists.


Pavel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hintak_leung at yahoo

Sep 3, 2011, 5:37 PM

Post #8 of 29 (1350 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

--- On Sun, 4/9/11, Pavel Ivanov <paivanof [at] gmail> wrote:

> On Sat, Sep 3, 2011 at 7:35 PM,
> >> So these 2 pointers are exactly what causing the
> trouble.
> >
> > This is interesting... so it would appear that
> hfsplus_read_wrapper() isn't working correctly, but enough
> of correct information to pass checks. I just re-read your
> e-mail and your device has a logical block size of 4096
> bytes, whereas most of the hfsplus code uses a block size of
> 512... you will need to look into hfsplus_submit_bio(),
> which is in the same file wrapper.c.
>
> 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.

> 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.

> > It is going to be difficult to do anything without the
> actual device and 8GB is too large to be send around.
> Assuming it is mostly music/media and there isn't too much
> stuff which is too confident/private, can I ask you to send
> me say, the first few MB of the disk?
>
> I'll send it in a separate email bypassing lists.

Thanks - I got them now. It will take a while to find my way around though...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 5, 2011, 9:35 PM

Post #9 of 29 (1340 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

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

>> 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?
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).

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.


Pavel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hintak_leung at yahoo

Sep 5, 2011, 10:12 PM

Post #10 of 29 (1341 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

--- On Tue, 6/9/11, Pavel Ivanov <paivanof [at] gmail> wrote:

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 6, 2011, 8:09 AM

Post #11 of 29 (1330 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Tue, Sep 6, 2011 at 1:12 AM, Hin-Tak Leung <hintak_leung [at] yahoo> wrote:
>> 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

I'd be happy to try them out and to help in testing/fixing/reviewing them.

>> 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.

Agreed. It was more of a quick hack to prove the correctness of such
change direction. I'll think how to do it better.


Thank you,
Pavel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


seth.forshee at canonical

Sep 7, 2011, 10:59 AM

Post #12 of 29 (1323 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

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 [at] yahoo> 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.


From d27825b880028e9a45ba640d86c9e8101db0606b Mon Sep 17 00:00:00 2001
From: Seth Forshee <seth.forshee [at] canonical>
Date: Wed, 7 Sep 2011 10:38:35 -0700
Subject: [PATCH] hfsplus: Fix kfree of wrong pointers in hfsplus_fill_super() error path

Commit 6596528 (hfsplus: ensure bio requests are not smaller than
the hardware sectors) changed the pointers used for volume header
allocations but failed to change the pointer freed in the error
path of hfsplus_fill_super(). This patch fixes the problem.

Reported-by: Pavel Ivanov <paivanof [at] gmail>
Signed-off-by: Seth Forshee <seth.forshee [at] canonical>
Cc: <stable [at] kernel>
---
fs/hfsplus/super.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index c106ca2..cadbb8c 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -525,8 +525,8 @@ out_close_cat_tree:
out_close_ext_tree:
hfs_btree_close(sbi->ext_tree);
out_free_vhdr:
- kfree(sbi->s_vhdr);
- kfree(sbi->s_backup_vhdr);
+ kfree(sbi->s_vhdr_buf);
+ kfree(sbi->s_backup_vhdr_buf);
out_unload_nls:
unload_nls(sbi->nls);
unload_nls(nls);
--
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


htl10 at users

Sep 7, 2011, 8:13 PM

Post #13 of 29 (1322 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

--- On Wed, 7/9/11, Seth Forshee <seth.forshee [at] canonical> wrote:

> 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.
>
>
> From d27825b880028e9a45ba640d86c9e8101db0606b Mon Sep 17
> 00:00:00 2001
> From: Seth Forshee <seth.forshee [at] canonical>
> Date: Wed, 7 Sep 2011 10:38:35 -0700
> Subject: [PATCH] hfsplus: Fix kfree of wrong pointers in
> hfsplus_fill_super() error path
>
> Commit 6596528 (hfsplus: ensure bio requests are not
> smaller than
> the hardware sectors) changed the pointers used for volume
> header
> allocations but failed to change the pointer freed in the
> error
> path of hfsplus_fill_super(). This patch fixes the
> problem.
>
> Reported-by: Pavel Ivanov <paivanof [at] gmail>
> Signed-off-by: Seth Forshee <seth.forshee [at] canonical>
> Cc: <stable [at] kernel>

Acked-by: Hin-Tak Leung <htl10 [at] users>

Please go ahead and submit the patch.

> ---
> fs/hfsplus/super.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index c106ca2..cadbb8c 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -525,8 +525,8 @@ out_close_cat_tree:
> out_close_ext_tree:
> hfs_btree_close(sbi->ext_tree);
> out_free_vhdr:
> - kfree(sbi->s_vhdr);
> - kfree(sbi->s_backup_vhdr);
> + kfree(sbi->s_vhdr_buf);
> + kfree(sbi->s_backup_vhdr_buf);
> out_unload_nls:
> unload_nls(sbi->nls);
> unload_nls(nls);
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-fsdevel" in
> the body of a message to majordomo [at] vger
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


seth.forshee at canonical

Sep 8, 2011, 9:23 AM

Post #14 of 29 (1312 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Thu, Sep 08, 2011 at 04:13:01AM +0100, Hin-Tak Leung wrote:
> --- On Wed, 7/9/11, Seth Forshee <seth.forshee [at] canonical> wrote:
>
> > 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.
> >
> >
> > From d27825b880028e9a45ba640d86c9e8101db0606b Mon Sep 17
> > 00:00:00 2001
> > From: Seth Forshee <seth.forshee [at] canonical>
> > Date: Wed, 7 Sep 2011 10:38:35 -0700
> > Subject: [PATCH] hfsplus: Fix kfree of wrong pointers in
> > hfsplus_fill_super() error path
> >
> > Commit 6596528 (hfsplus: ensure bio requests are not
> > smaller than
> > the hardware sectors) changed the pointers used for volume
> > header
> > allocations but failed to change the pointer freed in the
> > error
> > path of hfsplus_fill_super(). This patch fixes the
> > problem.
> >
> > Reported-by: Pavel Ivanov <paivanof [at] gmail>
> > Signed-off-by: Seth Forshee <seth.forshee [at] canonical>
> > Cc: <stable [at] kernel>
>
> Acked-by: Hin-Tak Leung <htl10 [at] users>
>
> Please go ahead and submit the patch.

That was my patch submission :)

Christoph, does that work for you, or would you like me to send the
patch in a separate email?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hch at infradead

Sep 9, 2011, 5:48 AM

Post #15 of 29 (1306 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Thu, Sep 08, 2011 at 09:23:04AM -0700, Seth Forshee wrote:
> That was my patch submission :)
>
> Christoph, does that work for you, or would you like me to send the
> patch in a separate email?

The submission is fine. I'll try to find a QA setup to test it properly
and will send it on to Linus afterwards.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 10, 2011, 8:32 PM

Post #16 of 29 (1286 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Thu, Sep 8, 2011 at 12:23 PM, Seth Forshee
<seth.forshee [at] canonical> wrote:
> On Thu, Sep 08, 2011 at 04:13:01AM +0100, Hin-Tak Leung wrote:
>> --- On Wed, 7/9/11, Seth Forshee <seth.forshee [at] canonical> wrote:
>>
>> > 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.
>> >
>> >
>> > From d27825b880028e9a45ba640d86c9e8101db0606b Mon Sep 17
>> > 00:00:00 2001
>> > From: Seth Forshee <seth.forshee [at] canonical>
>> > Date: Wed, 7 Sep 2011 10:38:35 -0700
>> > Subject: [PATCH] hfsplus: Fix kfree of wrong pointers in
>> > hfsplus_fill_super() error path
>> >
>> > Commit 6596528 (hfsplus: ensure bio requests are not
>> > smaller than
>> > the hardware sectors) changed the pointers used for volume
>> > header
>> > allocations but failed to change the pointer freed in the
>> > error
>> > path of hfsplus_fill_super(). This patch fixes the
>> > problem.
>> >
>> > Reported-by: Pavel Ivanov <paivanof [at] gmail>
>> > Signed-off-by: Seth Forshee <seth.forshee [at] canonical>
>> > Cc: <stable [at] kernel>
>>
>> Acked-by: Hin-Tak Leung <htl10 [at] users>
>>
>> Please go ahead and submit the patch.
>
> That was my patch submission :)
>
> Christoph, does that work for you, or would you like me to send the
> patch in a separate email?

Seth, Christoph,

I've just found another wrong pointers freeing. The following patch
should fix it.



Subject: [PATCH] hfsplus: Fix kfree of wrong pointers in
hfsplus_read_wrapper() error path

Commit 6596528 (hfsplus: ensure bio requests are not smaller than
the hardware sectors) changed the pointers used for volume header
allocations but failed to change the pointer freed in the error
path of hfsplus_read_wrapper(). This patch fixes the problem.

Signed-off-by: Pavel Ivanov <paivanof [at] gmail>
Cc: <stable [at] kernel>
---

diff --git a/fs/hfsplus/wrapper.c b/fs/hfsplus/wrapper.c
index 10e515a..7daf4b8 100644
--- a/fs/hfsplus/wrapper.c
+++ b/fs/hfsplus/wrapper.c
@@ -272,9 +272,9 @@ reread:
return 0;

out_free_backup_vhdr:
- kfree(sbi->s_backup_vhdr);
+ kfree(sbi->s_backup_vhdr_buf);
out_free_vhdr:
- kfree(sbi->s_vhdr);
+ kfree(sbi->s_vhdr_buf);
out:
return error;
}
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 10, 2011, 8:52 PM

Post #17 of 29 (1289 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Tue, Sep 6, 2011 at 11:09 AM, Pavel Ivanov <paivanof [at] gmail> wrote:
>>> 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;
>>
>> 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.
>
> Agreed. It was more of a quick hack to prove the correctness of such
> change direction. I'll think how to do it better.

Hin-Tak,

How about the following patch?


Subject: [PATCH] hfsplus: Allow to mount filesystem with block size
greater than PAGE_SIZE

Commit c6d5f5fa (hfsplus: lift the 2TB size limit) added call to
generic_check_addressable() but used the system's internal block size
which can be larger than PAGE_SIZE and will cause
generic_check_addressable() to return -EINVAL in this case. This
results in impossibility to mount file systems with such block sizes.
To fix it we have to use block size understandable by
generic_check_addressable() and adjust number of blocks accordingly.

Signed-off-by: Pavel Ivanov <paivanof [at] gmail>
Cc: <stable [at] kernel>
---

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index c106ca2..26ac5a5 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -399,8 +399,17 @@ 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 that filesystem data can be addressed by sector_t. Obvious way
+ * to do that is to call generic_check_addressable with alloc_blksz_shift
+ * and total_blocks as parameters. But alloc_blksz_shift can be greater
+ * than PAGE_SIZE_SHIFT and in this case it won't be accepted by
+ * generic_check_addressable. s_blocksize_bits will always be accepted
+ * and difference between it and alloc_blksz_shift will be always in
+ * fs_shift.
+ */
+ err = generic_check_addressable(sb->s_blocksize_bits,
+ (u64)sbi->total_blocks << sbi->fs_shift);
if (err) {
printk(KERN_ERR "hfs: filesystem size too large.\n");
goto out_free_vhdr;
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hintak_leung at yahoo

Sep 11, 2011, 6:46 AM

Post #18 of 29 (1283 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

--- On Sun, 11/9/11, Pavel Ivanov <paivanof [at] gmail> wrote:

> How about the following patch?
>
>
> Subject: [PATCH] hfsplus: Allow to mount filesystem with
> block size
> greater than PAGE_SIZE
>
> Commit c6d5f5fa (hfsplus: lift the 2TB size limit) added
> call to
> generic_check_addressable() but used the system's internal
> block size
> which can be larger than PAGE_SIZE and will cause
> generic_check_addressable() to return -EINVAL in this case.
> This
> results in impossibility to mount file systems with such
> block sizes.
> To fix it we have to use block size understandable by
> generic_check_addressable() and adjust number of blocks
> accordingly.
>
> Signed-off-by: Pavel Ivanov <paivanof [at] gmail>
> Cc: <stable [at] kernel>
> ---
>
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index c106ca2..26ac5a5 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -399,8 +399,17 @@ 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 that filesystem data
> can be addressed by sector_t. Obvious way
> +* to do that is to call
> generic_check_addressable with alloc_blksz_shift
> +* and total_blocks as
> parameters. But alloc_blksz_shift can be greater
> +* than PAGE_SIZE_SHIFT and
> in this case it won't be accepted by
> +* generic_check_addressable.
> s_blocksize_bits will always be accepted
> +* and difference between it
> and alloc_blksz_shift will be always in
> +* fs_shift.
> +*/
> + err =
> generic_check_addressable(sb->s_blocksize_bits,
> +
> (u64)sbi->total_blocks << sbi->fs_shift);
> if (err) {
> printk(KERN_ERR
> "hfs: filesystem size too large.\n");
> goto out_free_vhdr;

Why is the u64 needed? If it is needed, it probably means sbi->total_blocks should have been u64 in the first place.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hch at infradead

Sep 11, 2011, 7:10 AM

Post #19 of 29 (1280 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Sat, Sep 10, 2011 at 11:32:15PM -0400, Pavel Ivanov wrote:
> Seth, Christoph,
>
> I've just found another wrong pointers freeing. The following patch
> should fix it.

I'd like to throw this into Seths previous patch, while adding an
ACK for your finding. Is that fine with you?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hch at infradead

Sep 11, 2011, 7:12 AM

Post #20 of 29 (1282 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Sat, Sep 10, 2011 at 11:52:55PM -0400, Pavel Ivanov wrote:
> > Agreed. It was more of a quick hack to prove the correctness of such
> > change direction. I'll think how to do it better.
>
> Hin-Tak,
>
> How about the following patch?
>
>
> Subject: [PATCH] hfsplus: Allow to mount filesystem with block size
> greater than PAGE_SIZE
>
> Commit c6d5f5fa (hfsplus: lift the 2TB size limit) added call to
> generic_check_addressable() but used the system's internal block size
> which can be larger than PAGE_SIZE and will cause
> generic_check_addressable() to return -EINVAL in this case. This
> results in impossibility to mount file systems with such block sizes.
> To fix it we have to use block size understandable by
> generic_check_addressable() and adjust number of blocks accordingly.

Given that a large part of the checks are related to the actual
block size I think that we're better off just opencoding it.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hch at infradead

Sep 11, 2011, 7:14 AM

Post #21 of 29 (1281 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Sun, Sep 11, 2011 at 02:46:08PM +0100, Hin-Tak Leung wrote:
> > +?????* and difference between it
> > and alloc_blksz_shift will be always in
> > +?????* fs_shift.
> > +?????*/
> > +??? err =
> > generic_check_addressable(sb->s_blocksize_bits,
> > +??? ??? ???
> > (u64)sbi->total_blocks << sbi->fs_shift);
> > ??? if (err) {
> > ??? ??? printk(KERN_ERR
> > "hfs: filesystem size too large.\n");
> > ??? ??? goto out_free_vhdr;
>
> Why is the u64 needed? If it is needed, it probably means sbi->total_blocks should have been u64 in the first place.

No. When you shift values to the left they obviously become larger.
The total_blocks count on disk 32-bit so there is no need to store
it in a larger variable in the in-core superblock.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 12, 2011, 7:00 AM

Post #22 of 29 (1272 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Sun, Sep 11, 2011 at 10:10 AM, Christoph Hellwig <hch [at] infradead> wrote:
> On Sat, Sep 10, 2011 at 11:32:15PM -0400, Pavel Ivanov wrote:
>> Seth, Christoph,
>>
>> I've just found another wrong pointers freeing. The following patch
>> should fix it.
>
> I'd like to throw this into Seths previous patch, while adding an
> ACK for your finding. Is that fine with you?

Yes, that's fine with me.


Pavel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hch at infradead

Sep 12, 2011, 7:34 AM

Post #23 of 29 (1271 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

Does this patch fix your issues with large block sizes?


Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c 2011-09-12 09:56:58.619988416 -0400
+++ linux-2.6/fs/hfsplus/super.c 2011-09-12 10:07:18.006651395 -0400
@@ -344,6 +344,7 @@ static int hfsplus_fill_super(struct sup
struct inode *root, *inode;
struct qstr str;
struct nls_table *nls = NULL;
+ u64 last_fs_block, last_fs_page;
int err;

err = -EINVAL;
@@ -399,9 +400,13 @@ static int hfsplus_fill_super(struct sup
if (!sbi->rsrc_clump_blocks)
sbi->rsrc_clump_blocks = 1;

- err = generic_check_addressable(sbi->alloc_blksz_shift,
- sbi->total_blocks);
- if (err) {
+ err = -EFBIG;
+ last_fs_block = sbi->total_blocks - 1;
+ last_fs_page = (last_fs_block >> sbi->alloc_blksz_shift) <<
+ PAGE_CACHE_SHIFT;
+
+ if ((last_fs_block > (sector_t)(~0ULL) >> (sbi->alloc_blksz_shift - 9)) ||
+ (last_fs_page > (pgoff_t)(~0ULL))) {
printk(KERN_ERR "hfs: filesystem size too large.\n");
goto out_free_vhdr;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


paivanof at gmail

Sep 12, 2011, 8:19 AM

Post #24 of 29 (1274 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

On Mon, Sep 12, 2011 at 10:34 AM, Christoph Hellwig <hch [at] infradead> wrote:
> Does this patch fix your issues with large block sizes?

I'll be able to try it in the evening but meanwhile I have some comments below.

>
>
> Index: linux-2.6/fs/hfsplus/super.c
> ===================================================================
> --- linux-2.6.orig/fs/hfsplus/super.c 2011-09-12 09:56:58.619988416 -0400
> +++ linux-2.6/fs/hfsplus/super.c 2011-09-12 10:07:18.006651395 -0400
> @@ -344,6 +344,7 @@ static int hfsplus_fill_super(struct sup
> struct inode *root, *inode;
> struct qstr str;
> struct nls_table *nls = NULL;
> + u64 last_fs_block, last_fs_page;
> int err;
>
> err = -EINVAL;
> @@ -399,9 +400,13 @@ static int hfsplus_fill_super(struct sup
> if (!sbi->rsrc_clump_blocks)
> sbi->rsrc_clump_blocks = 1;
>
> - err = generic_check_addressable(sbi->alloc_blksz_shift,
> - sbi->total_blocks);
> - if (err) {
> + err = -EFBIG;
> + last_fs_block = sbi->total_blocks - 1;
> + last_fs_page = (last_fs_block >> sbi->alloc_blksz_shift) <<
> + PAGE_CACHE_SHIFT;

Did you mix left and right shifts here? Expression doesn't make sense to me.

Also I have a little concern about consistency in using
PAGE_CACHE_SHIFT and PAGE_SHIFT. hfsplus_read_wrapper() limits visible
block size to PAGE_SIZE, not PAGE_CACHE_SIZE. And although now they
are equal comment in linux/pagemap.h clearly says that PAGE_CACHE_SIZE
can be bigger than PAGE_SIZE. Is it something that should be fixed in
hfsplus_read_wrapper() ?

> +
> + if ((last_fs_block > (sector_t)(~0ULL) >> (sbi->alloc_blksz_shift - 9)) ||

Maybe this 9 should be extracted from here and
generic_check_addressable() into some macro?

> + (last_fs_page > (pgoff_t)(~0ULL))) {
> printk(KERN_ERR "hfs: filesystem size too large.\n");
> goto out_free_vhdr;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


hch at infradead

Sep 12, 2011, 8:31 AM

Post #25 of 29 (1274 views)
Permalink
Re: Kernel 3.1.0-rc4 oops when connecting iPod [In reply to]

Yes, the shifts were the wrong way around. I'll leave the 9 in for now,
we use it in so many places and if we're going to clean it up I'd do it
in ne go.

Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c 2011-09-12 11:20:33.866598100 -0400
+++ linux-2.6/fs/hfsplus/super.c 2011-09-12 11:20:52.538098052 -0400
@@ -344,6 +344,7 @@ static int hfsplus_fill_super(struct sup
struct inode *root, *inode;
struct qstr str;
struct nls_table *nls = NULL;
+ u64 last_fs_block, last_fs_page;
int err;

err = -EINVAL;
@@ -399,9 +400,13 @@ static int hfsplus_fill_super(struct sup
if (!sbi->rsrc_clump_blocks)
sbi->rsrc_clump_blocks = 1;

- err = generic_check_addressable(sbi->alloc_blksz_shift,
- sbi->total_blocks);
- if (err) {
+ err = -EFBIG;
+ last_fs_block = sbi->total_blocks - 1;
+ last_fs_page = (last_fs_block << sbi->alloc_blksz_shift) >>
+ PAGE_CACHE_SHIFT;
+
+ if ((last_fs_block > (sector_t)(~0ULL) >> (sbi->alloc_blksz_shift - 9)) ||
+ (last_fs_page > (pgoff_t)(~0ULL))) {
printk(KERN_ERR "hfs: filesystem size too large.\n");
goto out_free_vhdr;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

First page Previous page 1 2 Next page Last page  View All Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.