reiserfs NET=n build error

reiserfs NET=n build error

[Randy Dunlap]

With CONFIG_NET=n and REISERFS_FS=m (randconfig), kernel build ends with

Kernel: arch/x86_64/boot/bzImage is ready  (#15)
  Building modules, stage 2.
  MODPOST 137 modules
WARNING: "csum_partial" [fs/reiserfs/reiserfs.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

on both 2.6.19-rc6 and 2.6.19-rc6-git2.

Looks like arch/x86_64/lib/lib.a is not being linked into the
final kernel image for some reason.  lib.a does contain csum_partial.

---
~Randy
http://oss.oracle.com/~rdunlap/configs/config-rand-reiser-csum

Re: reiserfs NET=n build error

[Andi Kleen]

On Sunday 19 November 2006 05:22, Randy Dunlap wrote:
> With CONFIG_NET=n and REISERFS_FS=m (randconfig), kernel build ends with
> 
> Kernel: arch/x86_64/boot/bzImage is ready  (#15)
>   Building modules, stage 2.
>   MODPOST 137 modules
> WARNING: "csum_partial" [fs/reiserfs/reiserfs.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> 
> on both 2.6.19-rc6 and 2.6.19-rc6-git2.
> 
> Looks like arch/x86_64/lib/lib.a is not being linked into the
> final kernel image for some reason.  lib.a does contain csum_partial.

iirc Al Viro has been cleaning that up. Essentially reiserfs should
use its own C copy of the checksum functions. Using csum_partial() is not
safe because its output varies by architecture.

I would copy a relatively simple C implementation, like arch/h8300/lib/checksum.c

-Andi

Re: reiserfs NET=n build error

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andi Kleen wrote:
> On Sunday 19 November 2006 05:22, Randy Dunlap wrote:
>> With CONFIG_NET=n and REISERFS_FS=m (randconfig), kernel build ends with
>>
>> Kernel: arch/x86_64/boot/bzImage is ready  (#15)
>>   Building modules, stage 2.
>>   MODPOST 137 modules
>> WARNING: "csum_partial" [fs/reiserfs/reiserfs.ko] undefined!
>> make[1]: *** [__modpost] Error 1
>> make: *** [modules] Error 2
>>
>> on both 2.6.19-rc6 and 2.6.19-rc6-git2.
>>
>> Looks like arch/x86_64/lib/lib.a is not being linked into the
>> final kernel image for some reason.  lib.a does contain csum_partial.
> 
> iirc Al Viro has been cleaning that up. Essentially reiserfs should
> use its own C copy of the checksum functions. Using csum_partial() is not
> safe because its output varies by architecture.
> 
> I would copy a relatively simple C implementation, like arch/h8300/lib/checksum.c

As long as the h8300 version has the same output as the x86 version.

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFFYI/BLPWxlyuTD7IRAjveAJ0U+D3PO6h+2z83ZG9ZLay3q5JBFACcDHFQ
hIaiIBCFzfHg9qak9cxUtRo=
=GuqM
-----END PGP SIGNATURE-----

Re: reiserfs NET=n build error

[Andi Kleen]

> > I would copy a relatively simple C implementation, like arch/h8300/lib/checksum.c
> 
> As long as the h8300 version has the same output as the x86 version.

The trouble is that the different architecture have different output 
for csum_partial. So you already got a bug when someone wants to move
file systems.

-Andi

Re: reiserfs NET=n build error

[Randy Dunlap]

Andi Kleen wrote:
>>> I would copy a relatively simple C implementation, like arch/h8300/lib/checksum.c
>> As long as the h8300 version has the same output as the x86 version.
> 
> The trouble is that the different architecture have different output 
> for csum_partial. So you already got a bug when someone wants to move
> file systems.
> 
> -Andi

That argues for having only one version of it (in a lib.; my preference)
-or- Every module having its own local copy/version of it.  :(

-- 
~Randy

Re: reiserfs NET=n build error

[Al Viro]

On Sun, Nov 19, 2006 at 11:04:33AM -0800, Randy Dunlap wrote:
> Andi Kleen wrote:
> >>>I would copy a relatively simple C implementation, like 
> >>>arch/h8300/lib/checksum.c
> >>As long as the h8300 version has the same output as the x86 version.
> >
> >The trouble is that the different architecture have different output 
> >for csum_partial. So you already got a bug when someone wants to move
> >file systems.
> >
> >-Andi
> 
> That argues for having only one version of it (in a lib.; my preference)
> -or- Every module having its own local copy/version of it.  :(

Wrong.  csum_partial() result is defined modulo 0xffff and it's basically
"whatever's convenient as intermediate for this architecture".

reiserfs use of it is just plain broken.  net/* is fine, since all
final uses are via csum_fold() or equivalents.

Note that reiserfs use is broken in another way: it takes fixed-endian value
and feeds it to cpu_to_le32().  IOW, even if everything had literally the
same csum_partial(), the value it shits on disk would be endian-dependent.

As for net/*, with proper types it's pretty straightforward.  See
davem's net-2.6.20 for that...

Re: reiserfs NET=n build error

[Randy Dunlap]

On Sun, 19 Nov 2006 20:57:11 +0000 Al Viro wrote:

> On Sun, Nov 19, 2006 at 11:04:33AM -0800, Randy Dunlap wrote:
> > Andi Kleen wrote:
> > >>>I would copy a relatively simple C implementation, like 
> > >>>arch/h8300/lib/checksum.c
> > >>As long as the h8300 version has the same output as the x86 version.
> > >
> > >The trouble is that the different architecture have different output 
> > >for csum_partial. So you already got a bug when someone wants to move
> > >file systems.
> > >
> > >-Andi
> > 
> > That argues for having only one version of it (in a lib.; my preference)
> > -or- Every module having its own local copy/version of it.  :(
> 
> Wrong.  csum_partial() result is defined modulo 0xffff and it's basically
> "whatever's convenient as intermediate for this architecture".
> 
> reiserfs use of it is just plain broken.  net/* is fine, since all
> final uses are via csum_fold() or equivalents.
> 
> Note that reiserfs use is broken in another way: it takes fixed-endian value
> and feeds it to cpu_to_le32().  IOW, even if everything had literally the
> same csum_partial(), the value it shits on disk would be endian-dependent.
> 
> As for net/*, with proper types it's pretty straightforward.  See
> davem's net-2.6.20 for that...

OK.  I guessed that there was some sanity in there somewhere (either
in the provider or consumer), but apparently there's none in either place.
:(

---
~Randy

Re: reiserfs NET=n build error

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Al Viro wrote:
> On Sun, Nov 19, 2006 at 11:04:33AM -0800, Randy Dunlap wrote:
>> Andi Kleen wrote:
>>>>> I would copy a relatively simple C implementation, like 
>>>>> arch/h8300/lib/checksum.c
>>>> As long as the h8300 version has the same output as the x86 version.
>>> The trouble is that the different architecture have different output 
>>> for csum_partial. So you already got a bug when someone wants to move
>>> file systems.
>>>
>>> -Andi
>> That argues for having only one version of it (in a lib.; my preference)
>> -or- Every module having its own local copy/version of it.  :(
> 
> Wrong.  csum_partial() result is defined modulo 0xffff and it's basically
> "whatever's convenient as intermediate for this architecture".
> 
> reiserfs use of it is just plain broken.  net/* is fine, since all
> final uses are via csum_fold() or equivalents.
> 
> Note that reiserfs use is broken in another way: it takes fixed-endian value
> and feeds it to cpu_to_le32().  IOW, even if everything had literally the
> same csum_partial(), the value it shits on disk would be endian-dependent.

Oh great. Even better. :(

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFFYNtqLPWxlyuTD7IRAux8AKCbxW4zX5Q7y8LfPT0FY/W4A8v0PQCggV11
EbMvTGkAb5WXa0f7EgUz5Qk=
=Zm0q
-----END PGP SIGNATURE-----

Re: reiserfs NET=n build error

[Randy Dunlap]

On Sun, 19 Nov 2006 17:32:11 -0500 Jeff Mahoney wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Al Viro wrote:
> > On Sun, Nov 19, 2006 at 11:04:33AM -0800, Randy Dunlap wrote:
> >> Andi Kleen wrote:
> >>>>> I would copy a relatively simple C implementation, like 
> >>>>> arch/h8300/lib/checksum.c
> >>>> As long as the h8300 version has the same output as the x86 version.
> >>> The trouble is that the different architecture have different output 
> >>> for csum_partial. So you already got a bug when someone wants to move
> >>> file systems.
> >>>
> >>> -Andi
> >> That argues for having only one version of it (in a lib.; my preference)
> >> -or- Every module having its own local copy/version of it.  :(
> > 
> > Wrong.  csum_partial() result is defined modulo 0xffff and it's basically
> > "whatever's convenient as intermediate for this architecture".
> > 
> > reiserfs use of it is just plain broken.  net/* is fine, since all
> > final uses are via csum_fold() or equivalents.
> > 
> > Note that reiserfs use is broken in another way: it takes fixed-endian value
> > and feeds it to cpu_to_le32().  IOW, even if everything had literally the
> > same csum_partial(), the value it shits on disk would be endian-dependent.
> 
> Oh great. Even better. :(

Even more:  MD/raid (=m) is broken in this way also.
It uses csum_partial(), which isn't present (CONFIG_NET=n).

Kernel: arch/x86_64/boot/bzImage is ready  (#9)
  Building modules, stage 2.
  MODPOST 101 modules
WARNING: "csum_partial" [drivers/md/md-mod.ko] undefined!
make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2

CONFIG_MD=y
CONFIG_BLK_DEV_MD=m
CONFIG_MD_LINEAR=m
# CONFIG_MD_RAID0 is not set
CONFIG_MD_RAID1=m
# CONFIG_MD_RAID10 is not set
# CONFIG_MD_RAID456 is not set
CONFIG_MD_MULTIPATH=m
# CONFIG_MD_FAULTY is not set
CONFIG_BLK_DEV_DM=y
CONFIG_DM_DEBUG=y
CONFIG_DM_CRYPT=m
CONFIG_DM_SNAPSHOT=y
CONFIG_DM_MIRROR=y
CONFIG_DM_ZERO=m
CONFIG_DM_MULTIPATH=y
CONFIG_DM_MULTIPATH_EMC=m

---
~Randy
full broken config for MD:
http://oss.oracle.com/~rdunlap/configs/config-md-csum

Re: reiserfs NET=n build error

[Randy Dunlap]

On Tue, 28 Nov 2006 11:47:57 -0800 Randy Dunlap wrote:

> On Sun, 19 Nov 2006 17:32:11 -0500 Jeff Mahoney wrote:
> 
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > Al Viro wrote:
> > > On Sun, Nov 19, 2006 at 11:04:33AM -0800, Randy Dunlap wrote:
> > >> Andi Kleen wrote:
> > >>>>> I would copy a relatively simple C implementation, like 
> > >>>>> arch/h8300/lib/checksum.c
> > >>>> As long as the h8300 version has the same output as the x86 version.
> > >>> The trouble is that the different architecture have different output 
> > >>> for csum_partial. So you already got a bug when someone wants to move
> > >>> file systems.
> > >>>
> > >>> -Andi
> > >> That argues for having only one version of it (in a lib.; my preference)
> > >> -or- Every module having its own local copy/version of it.  :(
> > > 
> > > Wrong.  csum_partial() result is defined modulo 0xffff and it's basically
> > > "whatever's convenient as intermediate for this architecture".
> > > 
> > > reiserfs use of it is just plain broken.  net/* is fine, since all
> > > final uses are via csum_fold() or equivalents.
> > > 
> > > Note that reiserfs use is broken in another way: it takes fixed-endian value
> > > and feeds it to cpu_to_le32().  IOW, even if everything had literally the
> > > same csum_partial(), the value it shits on disk would be endian-dependent.
> > 
> > Oh great. Even better. :(
> 
> Even more:  MD/raid (=m) is broken in this way also.
> It uses csum_partial(), which isn't present (CONFIG_NET=n).
> 
> Kernel: arch/x86_64/boot/bzImage is ready  (#9)
>   Building modules, stage 2.
>   MODPOST 101 modules
> WARNING: "csum_partial" [drivers/md/md-mod.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> 
> CONFIG_MD=y
> CONFIG_BLK_DEV_MD=m
> CONFIG_MD_LINEAR=m
> # CONFIG_MD_RAID0 is not set
> CONFIG_MD_RAID1=m
> # CONFIG_MD_RAID10 is not set
> # CONFIG_MD_RAID456 is not set
> CONFIG_MD_MULTIPATH=m
> # CONFIG_MD_FAULTY is not set
> CONFIG_BLK_DEV_DM=y
> CONFIG_DM_DEBUG=y
> CONFIG_DM_CRYPT=m
> CONFIG_DM_SNAPSHOT=y
> CONFIG_DM_MIRROR=y
> CONFIG_DM_ZERO=m
> CONFIG_DM_MULTIPATH=y
> CONFIG_DM_MULTIPATH_EMC=m
> 
> ---
> ~Randy
> full broken config for MD:
> http://oss.oracle.com/~rdunlap/configs/config-md-csum

Ingo, Neil:

Al has summarized that csum_partial() is arch-specific.
However, drivers/md/md.c uses it.

Does that mean that RAID volumes are not portable across
(some) architectures?

Should md.c use a specific, known, fixed (as in static,
arch-independent) version of csum_partial()?

Will changing now possibly make some existing volumes
non-portable?

Thanks,
---
~Randy

Re: reiserfs NET=n build error

[Neil Brown]

On Monday December 4, randy.dunlap@oracle.com wrote:
> 
> Ingo, Neil:
> 
> Al has summarized that csum_partial() is arch-specific.
> However, drivers/md/md.c uses it.

Yep.

> 
> Does that mean that RAID volumes are not portable across
> (some) architectures?

Yep.  version-0.90 superblocks are already not portable between archs
of different endianness.  There can also be issues between arch with
different implementations of csum_partial, though the use of csum_fold
in
	if (csum_fold(calc_sb_csum(sb)) != csum_fold(sb->sb_csum)) {
		printk(KERN_WARNING "md: invalid superblock checksum on %s\n",
			b);
(in super_90_load in md.c) tries to alleviate this.

> 
> Should md.c use a specific, known, fixed (as in static,
> arch-independent) version of csum_partial()?

For version-1 superblocks it uses arch-independent byte-order and
arch-independent checksums but....

> 
> Will changing now possibly make some existing volumes
> non-portable?

.. it really is too late for 0.90 superblocks.  Certainly changing it
would back things for people who want to revert to an earlier kernel.

The use of csum_fold has been in place since late 2004 so you would
need to go quite a long way back to hit problems... and if you go that
far back you could hit problems with mdadm too (as mdadm calculated
the checksum the same on all architectures...).

So maybe we could get rid of csum_partial and use a replacement and
still have most things work.... tested patched would be considered :-)

NeilBrown

Re: reiserfs NET=n build error

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andi Kleen wrote:
>>> I would copy a relatively simple C implementation, like arch/h8300/lib/checksum.c
>> As long as the h8300 version has the same output as the x86 version.
> 
> The trouble is that the different architecture have different output 
> for csum_partial. So you already got a bug when someone wants to move
> file systems.

Yeah, Al Viro noticed that about reiserfs earlier this month. The
problem is that there's really no good fix for it. I was under the
impression that csum_partial would be arch-independent and was in asm/
for performance reasons. The comment in asm-x86_64 indicates that's not
the case, but the comment in asm-i386 still doesn't. I developed the
code on i386. Moving forward we can define an arch-independent hash
function for that and accept the old arch-dependent checksums, but
there's still the issue of old kernels not understanding it on any arch.
Kind of a nice shot to the foot considering the work I put into making
reiserfs endian safe in the 2.4 days.

I'm hoping there's a better solution to be found than creating a
checksum verifier that checks all known versions. :(


- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFFYLqvLPWxlyuTD7IRAj7RAKCkOHL9EgTrmHSo97xzG5tBxWgzCACgiBcW
uzd/oSwXDHECHPEcIL58xoo=
=udEd
-----END PGP SIGNATURE-----

Re: reiserfs NET=n build error

[Al Viro]

On Sun, Nov 19, 2006 at 12:09:22PM -0500, Jeff Mahoney wrote:
> > I would copy a relatively simple C implementation, like arch/h8300/lib/checksum.c
> 
> As long as the h8300 version has the same output as the x86 version.

As the matter of fact, h8300 version is severely broken and no, it
definitely does *not* give the same values.  Not even guaranteed
to be the same mod 0xffff is some cases, which is very much not
OK for networking uses.

Re: reiserfs NET=n build error

[Adrian Bunk]

On Sat, Nov 18, 2006 at 08:22:06PM -0800, Randy Dunlap wrote:
> With CONFIG_NET=n and REISERFS_FS=m (randconfig), kernel build ends with
> 
> Kernel: arch/x86_64/boot/bzImage is ready  (#15)
>   Building modules, stage 2.
>   MODPOST 137 modules
> WARNING: "csum_partial" [fs/reiserfs/reiserfs.ko] undefined!
> make[1]: *** [__modpost] Error 1
> make: *** [modules] Error 2
> 
> on both 2.6.19-rc6 and 2.6.19-rc6-git2.
> 
> Looks like arch/x86_64/lib/lib.a is not being linked into the
> final kernel image for some reason.  lib.a does contain csum_partial.

The bug is in arch/x86_64/lib/Makefile:

The problem is that lib-y objects only get linked into the kernel image 
when they are used in the kernel image.

Therefore, an EXPORT_SYMBOL in a lib-y object is a bug.

> ~Randy

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [discuss] Re: reiserfs NET=n build error

[Andi Kleen]

 
> Therefore, an EXPORT_SYMBOL in a lib-y object is a bug.


True. I think that got added when the EXPORT_SYMBOL was moved
into the individual .c file.

It would be better to move it back into some other file inside
a #ifdef CONFIG_NET. And fix reiserfs to not use it because
it shouldn't anyways.

-Andi