gcc 11 cleanup

[AttilaFueloep]

Motivation and Context

Building with gcc 11.1.0 fails due to warnings.

Description

Disable false positive warnings, fix a wrong prototype.

Closes #12130
Closes #12188

How Has This Been Tested?

Just a local build.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[AttilaFueloep]

Not sure if the #pragma approach is acceptable, if not please advice.

@felixdoerre Shouldn't we check the return value of mlock() in contrib/pam_zfs_key/pam_zfs_key.c?

[felixdoerre]

Yes, probably we should treat a failure in mlock like an allocation failure and just free pw->value and pw and return NULL.

[AttilaFueloep]

A bit more detail.

Not sure where the warning in contrib/pam_zfs_key/pam_zfs_key.c is comming from, the code indicates that pw can't be used uninitialized. Neither casting the return value to (void) nor casting the first argument of mlock() to (const void *) got rid of the warning.

pam_zfs_key.c: In function ‘alloc_pw_string’:
pam_zfs_key.c:107:16: error: ‘<unknown>’ may be used uninitialized [-Werror=maybe-uninitialized]
  107 |         (mlock(pw->value, pw->len);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../include/sys/zfs_context.h:106,
                 from ../../include/sys/dmu.h:45,
                 from ../../include/sys/dmu_tx.h:33,
                 from ../../include/sys/dsl_crypt.h:23,
                 from pam_zfs_key.c:28:
/usr/include/sys/mman.h:103:12: note: by argument 1 of type ‘const void *’ to ‘mlock’ declared here
  103 | extern int mlock (const void *__addr, size_t __len) __THROW;
      |            ^~~~~

The extern declaration in include/sys/crypto/api.h does not match the definition in module/icp/api/kcf_misc_api.c

../../module/icp/api/kcf_miscapi.c:65:22: error: argument 1 of type ‘char *’ declared as a pointer [-Werror=array-parameter=]
   65 | crypto_mech2id(char *mechname)
      |                ~~~~~~^~~~~~~~
In file included from ../../module/icp/api/kcf_miscapi.c:28:
../../include/sys/crypto/api.h:61:61: note: previously declared as an array ‘char[32]’
   61 | extern crypto_mech_type_t crypto_mech2id(crypto_mech_name_t name);
      |                                          ~~~~~~~~~~~~~~~~~~~^~~~

The warnings in module/os/linux/zfs/zio_crypt.c stem from the fact that we truncated the dnode to 64 bytes to get rid of the non-portable union at the end. Since we don't access the union it's a false positive.

../../module/os/linux/zfs/zio_crypt.c:1060:54: error: array subscript ‘dnode_phys_t {aka struct dnode_phys}[0]’ is partly outside array bounds of ‘uint8_t[64]’ {aka ‘unsigned char[64]’} [-Werror=array-bounds]
 1060 |                 adnp->dn_datablkszsec = BSWAP_16(adnp->dn_datablkszsec);
      |                                                      ^~
...

[AttilaFueloep]

@felixdoerre All right, will open a PR once this is merged.

[dioni21]

Aren't we just hiding the real problems with the above patch? I can also compile if I simply disable the -Werror option, but the warning may have something important to say.

In particular, I do not like the idea of using a generic char * when we have a better restrained type crypto_mech_name_t

[dioni21]

BTW: You do not have to make another PR, just add new commits (even forced ones) to the same branch (AttilaFueloep:zfs-icp-gcc11).

[AttilaFueloep]

@dioni21

Aren't we just hiding the real problems with the above patch?

zfs/module/icp/api/kcf_miscapi.c

Lines 44 to 49 in 83ba91a

	/*
	* crypto_mech2id()
	*
	* Arguments:
	* . mechname: A null-terminated string identifying the mechanism name.
	*

So a char * is adequate. It is cast to a void * later on anyhow.

BTW: You do not have to make another PR

PRs should be limited to fixing distinct problems.

[behlendorf]

Not sure if the #pragma approach is acceptable, if not please advice.

As a general rule we have tried to avoid using #pragma GCC diagnostic and have only fallen back to it as a last resort (which was needed in a few cases).

I briefly looked at the alloc_pw_string() function and I'm not sure why gcc is claiming it can be used uninitialized. That doesn't look possible to me, perhaps the code could be changed slightly so it gets the analysis right.

[AttilaFueloep]

Given the weird location of the arrow (^~~~) at mlock and the error: ‘<unknown>’ text, I've the impression that something else is going on. I'll try to shuffle the code a bit, let's see if it helps. That leaves the truncated dnode problem, not sure what to change there to get gcc happy.

[AttilaFueloep]

That leaves the truncated dnode problem,

Well, I could introduce a new struct, say dnode_trunc and cast the array to that. Not sure if this is worthwhile though.

[AttilaFueloep]

Managed to get rid of the #pragmas. Reshuffling the code in pam_zfs_key.c didn't help, I ended up zeroing pw->value before calling mlock() on it. I think this shouldn't be too bad.

[dioni21]

Reshuffling the code in pam_zfs_key.c didn't help, I ended up zeroing pw->value before calling mlock() on it. I think this shouldn't be too bad.

Not bad, but hurts my brain optimizer. :-)

Did you try a cast to (const void *) when calling mlock?

Or, at least, use calloc instead of malloc + memset.

And in extern crypto_mech_type_t crypto_mech2id(char *name); I would also use const char *

Anyway, this may be just me doing yet another bikeshedding. :-(

Thanks for your work, @AttilaFueloep !

[AttilaFueloep]

@dioni21

Did you try a cast to (const void *) when calling mlock?

Yep.

Or, at least, use calloc instead of malloc + memset.

Yeah, good idea.

And in extern crypto_mech_type_t crypto_mech2id(char *name); I would also use const char *

While I generally agree, this would involve refactoring the whole call chain, which I think is outside the scope of this PR.

[ahrens]

It seems a little unfortunate to have to duplicate this. Could we embed it in the dnode_phys_t instead? Though I guess since it's on-disk it won't be changing very often.

[AttilaFueloep]

Yes, that was my concern and reasoning as well. I do type embedding frequently in Go but I failed to figure how to do it in C without requiring Microsoft extensions (gcc/clang -fms-extensions flag). Even in C11 you can't have an anonymous struct member of a previously defined type. Am I missing something obvious here? Or are you thinking of doing some #define ?

[ahrens]

I think the cleanest way is to do something like

struct dnode_phys {
    dnode_phys_core_t dn_core;
    union {
        ...
   }
}

The downside is that you have to change all the code that's accessing it to add .dn_core :( . (And the "solution" to that of doing #define dn_type dn_core.dn_type seems worse.)

I'm guessing that the compiler's complaint about the current code is that you have a dnode_phys_t* that points to an array that is not large enough to hold a dnode_phys_t. I think the proposed solution is decent but I wonder if there's another way to do this. Maybe if we allocated a larger buffer to point to (might have to be on the heap due to the linux kernel's limited stack space). That would be a little slower but it might not be significantly so.

[AttilaFueloep]

The downside is that you have to change all the code that's accessing it to add .dn_core :( . (And the "solution" to that of doing #define dn_type dn_core.dn_type seems worse.)

Yeah, fully agree. It would be so nice if we could just say

struct dnode_phys {
    struct dnode_phys_core;
    union {
        ...
   }
}

but well, maybe in ten years, using C23, that'll work. ;-)

I'm guessing that the compiler's complaint about the current code is that you have a dnode_phys_t* that points to an array that is not large enough to hold a dnode_phys_t.

Exactly, ‘dnode_phys_t {aka struct dnode_phys}[0]’ is partly outside array bounds. The full warning is here at the bottom.

Maybe if we allocated a larger buffer to point to

That didn't occur to me, but your right, using a dnode_phys_t and just limiting the HMAC calculation to the first 64 bytes of that may be a cleaner solution. IIRC the stack size in Linux is 16k and a dnode_phys_t is 512 bytes, so limited stack space shouldn't be a concern. The only drawbacks I can see is 448 more bytes on the stack and maybe slightly less clearer code. I'll push a changed version. Which approach to choose I leave up to you, I'm fine either way.

[behlendorf]

It's unfortunate we need to use the additional space on the stack for this, but I agree it shouldn't be a problem here. Things are a bit tighter on i386 with uses 8k stacks, but I believe we'll still be OK there as well. This does seem like the cleanest way to handle this to me.

[AttilaFueloep]

All right. I'll hold-up squashing until a second reviewer had a chance to look at it.

[ahrens]

Thanks for doing that, looks great!

[AttilaFueloep]

My pleasure. Squashed, rebased and updated commit comment.