nvpair: inconsistent XDP packing of char types

[brooksdavis]

System information

Type	Version/Name
Distribution Name	FreeBSD/CheriBSD
Distribution Version	13.1/22.05+
Kernel Version	13.1/22.05+
Architecture	amd64/aarch64
OpenZFS Version	HEAD (945b407 plus a bunch of local patches)

Describe the problem you're observing

The XDR packed output of 8-bit integer types is inconstant between architectures where char is signed (amd64, sparc) and ones where it is unsigned (aarch64). Specifically values greater than 127 are stored sign-extended on signed char architectures and zero-extended on unsigned char architectures. This is because all of them are encoded using xdr_char() which is (on FreeBSD which uses the Sun RPC implementation):

bool_t
xdr_char(XDR *xdrs, char *cp)
{
        int i;

        i = (*cp);
        if (!xdr_int(xdrs, &i)) {   
                return (FALSE);
        }
        *cp = i;
        return (TRUE);
}

It's arguably a bug that uint8 (uint8_t) values are being encoded this way. It's likewise probably a bug that int8 (int8_t) types aren't being sign extended on signed architectures. Unfortunately there is no portable xdr_s_char() or xdr_int8() to go with xdr_u_char() so we'd likely need to implement one.

Describe how to reproduce the problem

On an aarch64 (or other unsigned char) system with #14160 applied to your openzfs tree, do a build and then, run:

$ tests/zfs-tests/cmd/nvlist_packed -v -r tests/zfs-tests/tests/functional/libnvpair/refs -a

You'll see the int8_array and uint8_array cases fail.

You can examine the difference in the packed output by creating an alternative set of reference files:

$ mkdir bad-refs
$ tests/zfs-tests/cmd/nvlist_packed -r bad-refs/ -R -a
$ hd bad-refs/int8_array.ref
00000000  01 01 00 00 00 00 00 00  00 00 00 00 00 00 00 34  |...............4|
00000010  00 00 00 28 00 00 00 0a  69 6e 74 38 5f 61 72 72  |...(....int8_arr|
00000020  61 79 00 00 00 00 00 19  00 00 00 04 00 00 00 04  |ay..............|
00000030  00 00 00 00 00 00 00 01  00 00 00 02 00 00 00 ff  |................|
00000040  00 00 00 00 00 00 00 00                           |........|
00000048
$ hd tests/zfs-tests/tests/functional/libnvpair/refs/int8_array.ref
00000000  01 01 00 00 00 00 00 00  00 00 00 00 00 00 00 34  |...............4|
00000010  00 00 00 28 00 00 00 0a  69 6e 74 38 5f 61 72 72  |...(....int8_arr|
00000020  61 79 00 00 00 00 00 19  00 00 00 04 00 00 00 04  |ay..............|
00000030  00 00 00 00 00 00 00 01  00 00 00 02 ff ff ff ff  |................|
00000040  00 00 00 00 00 00 00 00                           |........|
00000048

These are both XDR streams holding a 4-element int8_array with contents {0,1,2,-1}. With the one in bad-refs being generated on an aarch64 system. The difference occurs in the last word of the line labeled 00000030.

[brooksdavis]

The question now is, what to do about this? I think the difference is harmless and the results are still compatible (but lack complete confidence in my knowledge of the C language math edge cases here). If so or if none of the various uses of nvlist_{,u}int8{,_array}() make it to disk, the right thing to do is probably to fix the encoded to sign extend or not appropriately and update #14160 to match.

[brooksdavis]

After some discussion with @ahrens we've concluded this is a bug in the FreeBSD implementation of xdr_char(). The Illumos version casts through unsigned char:

static bool_t
xdrmem_enc_char(XDR *xdrs, char *cp)
{
	uint32_t val;

	BUILD_BUG_ON(sizeof (char) != 1);
	val = *((unsigned char *) cp);

	return (xdrmem_enc_uint32(xdrs, val));
}

[brooksdavis]

I've now committed a fix to FreeBSD main as well as stable/12 and stable/13. It will appear in FreeBSD 13.1 and 14.0.