From: Hin-Tak Leung <hintak_leung@yahoo.co.uk> To: Pavel Ivanov <paivanof@gmail.com> Cc: linux-fsdevel@vger.kernel.org, linux-kernel <linux-kernel@vger.kernel.org>, Seth Forshee <seth.forshee@canonical.com>, Christoph Hellwig <hch@tuxera.com> Subject: Re: Kernel 3.1.0-rc4 oops when connecting iPod Date: Tue, 6 Sep 2011 06:12:01 +0100 (BST) [thread overview] Message-ID: <1315285921.46647.YahooMailClassic@web29519.mail.ird.yahoo.com> (raw) In-Reply-To: <CAG1a4ruG8bL9j01RgSXuK2w9FV0izp8vdoQMLEZzc=qZfCNpzA@mail.gmail.com> --- 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.
WARNING: multiple messages have this Message-ID (diff)
From: Hin-Tak Leung <hintak_leung@yahoo.co.uk> To: Pavel Ivanov <paivanof@gmail.com> Cc: linux-fsdevel@vger.kernel.org, linux-kernel <linux-kernel@vger.kernel.org>, Seth Forshee <seth.forshee@canonical.com>, Christoph Hellwig <hch@tuxera.com> Subject: Re: Kernel 3.1.0-rc4 oops when connecting iPod Date: Tue, 6 Sep 2011 06:12:01 +0100 (BST) [thread overview] Message-ID: <1315285921.46647.YahooMailClassic@web29519.mail.ird.yahoo.com> (raw) In-Reply-To: <CAG1a4ruG8bL9j01RgSXuK2w9FV0izp8vdoQMLEZzc=qZfCNpzA@mail.gmail.com> --- 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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2011-09-06 5:12 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-09-03 3:08 Kernel 3.1.0-rc4 oops when connecting iPod Pavel Ivanov 2011-09-03 3:59 ` Hin-Tak Leung 2011-09-03 3:59 ` Hin-Tak Leung 2011-09-03 4:37 ` Pavel Ivanov 2011-09-03 6:57 ` Hin-Tak Leung 2011-09-03 20:52 ` Pavel Ivanov 2011-09-03 20:52 ` Pavel Ivanov 2011-09-03 23:35 ` Hin-Tak Leung 2011-09-03 23:35 ` Hin-Tak Leung 2011-09-03 23:59 ` Pavel Ivanov 2011-09-03 23:59 ` Pavel Ivanov 2011-09-04 0:37 ` Hin-Tak Leung 2011-09-06 4:35 ` Pavel Ivanov 2011-09-06 4:35 ` Pavel Ivanov 2011-09-06 5:12 ` Hin-Tak Leung [this message] 2011-09-06 5:12 ` Hin-Tak Leung 2011-09-06 15:09 ` Pavel Ivanov 2011-09-06 15:09 ` Pavel Ivanov 2011-09-11 3:52 ` Pavel Ivanov 2011-09-11 3:52 ` Pavel Ivanov 2011-09-11 13:46 ` Hin-Tak Leung 2011-09-11 13:46 ` Hin-Tak Leung 2011-09-11 14:14 ` Christoph Hellwig 2011-09-11 14:12 ` Christoph Hellwig 2011-09-12 14:34 ` Christoph Hellwig 2011-09-12 15:19 ` Pavel Ivanov 2011-09-12 15:19 ` Pavel Ivanov 2011-09-12 15:31 ` Christoph Hellwig 2011-09-13 2:20 ` Pavel Ivanov 2011-09-13 2:20 ` Pavel Ivanov 2011-09-14 17:42 ` Christoph Hellwig 2011-09-15 14:35 ` Christoph Hellwig 2011-09-12 15:57 ` Hin-Tak Leung 2011-09-07 17:59 ` Seth Forshee 2011-09-07 17:59 ` Seth Forshee 2011-09-08 3:13 ` Hin-Tak Leung 2011-09-08 3:13 ` Hin-Tak Leung 2011-09-08 16:23 ` Seth Forshee 2011-09-09 12:48 ` Christoph Hellwig 2011-09-11 3:32 ` Pavel Ivanov 2011-09-11 3:32 ` Pavel Ivanov 2011-09-11 14:10 ` Christoph Hellwig 2011-09-12 14:00 ` Pavel Ivanov 2011-09-12 15:27 ` [PATCH] hfsplus: Fix kfree of wrong vhdr pointers Seth Forshee 2011-09-12 15:28 ` Christoph Hellwig
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1315285921.46647.YahooMailClassic@web29519.mail.ird.yahoo.com \ --to=hintak_leung@yahoo.co.uk \ --cc=hch@tuxera.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=paivanof@gmail.com \ --cc=seth.forshee@canonical.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.