zgenhostid: accept hostid arguments equal to zero. #11189

ericonr · 2020-11-10T14:53:36Z

Motivation and Context

A common usage pattern for zgenhostid, including in the ZFS dracut
module, is running it as:

zgenhostid $(hostid)

However, zgenhostid only accepted hostid arguments greater than 0, which
meant that, when the output of hostid(1) was "00000000", zgenhostid
would error out, even though 0 is a possible return value for the
gethostid(3) function used by hostid(1):

On current musl libc, gethostid(3) is a stub that always returns 0.
On glibc, gethostid(3) will return 0 if /etc/hostid exists but is
smaller than 4 bytes.

Fixes #11174

I should note that the current mmp_set_hostid test in tests/zfs-tests/tests/functional/mmp/mmp.kshlib is simply broken on musl-based systems, due to using the hostid(1) utility. An alternative to this PR is extending zgenhostid to read /etc/hostid by itself instead of relying on the external hostid utility, since zfs already avoids using libc's gethostid(3) in libspl.

Description

In these cases, it makes more sense for zgenhostid to treat a value of 0
as other parts of the zfs codebase do, meaning that a hostid value
couldn't be determined; therefore, it should attempt to generate a
random value to write into /etc/hostid.

How Has This Been Tested?

I ran it as zgenhostid -o /tmp/hostid 00000000 and /tmp/hostid was now populated with random values.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

behlendorf · 2020-11-10T18:47:04Z

@gyakovlev would you mind looking at this change to zgenhostid. Given the described use case this sounds pretty reasonable to me.

A common usage pattern for zgenhostid, including in the ZFS dracut module, is running it as: zgenhostid $(hostid) However, zgenhostid only accepted hostid arguments greater than 0, which meant that, when the output of hostid(1) was "00000000", zgenhostid would error out, even though 0 is a possible return value for the gethostid(3) function used by hostid(1): - On current musl libc, gethostid(3) is a stub that always returns 0. - On glibc, gethostid(3) will return 0 if /etc/hostid exists but is smaller than 4 bytes. In these cases, it makes more sense for zgenhostid to treat a value of 0 as other parts of the zfs codebase do, meaning that a hostid value couldn't be determined; therefore, it should attempt to generate a random value to write into /etc/hostid. The manpage and usage output have been updated to reflect this. Whitespace has also been fixed in the usage output. Closes openzfs#11174 Signed-off-by: Érico Rolim <[email protected]>

ericonr · 2020-11-11T06:00:57Z

@behlendorf I updated the PR to include a couple small fixes in the usage function, both to document the new behavior and fix an issue with whitespace. I hope that's okay to include in this PR.

gyakovlev

thanks,
it makes sense.
code and doc changes look good to me. tested briefly. works as expected if hostid binary returns all zeroes and it's output passed to zgenhostid, it properly generates random one.

I only tested zgenhostid generation on musl/glibc, but not actual dracut.
if that's enough to fix musl, it should be ok.

documentation mentions just 0, so maybe docs should mention that user needs to pass 00000000 or 0x00000000 explicitly?
Sure, previous sentence mentions length requirements, but providing just 0 as example value may be a bit confusing.

ericonr · 2020-11-11T13:16:43Z

documentation mentions just 0, so maybe docs should mention that user needs to pass 00000000 or 0x00000000 explicitly?
Sure, previous sentence mentions length requirements, but providing just 0 as example value may be a bit confusing.

The other values are shown as 1 and 2^32-1, so I thought it would be best to stick with the same style. Not using 0 feels inconsistent imho, but I can change it.

ericonr · 2020-11-11T13:28:28Z

I have tested dracut as well by replacing the binary in /usr/bin, and it worked fine in that it didn't show any error messages.

However, as far as I can tell, most musl users should be advised to manually copy the hexadecimal representation of /etc/hostid to spl_hostid= in the kernel command line; or possibly the dracut module should be changed to simply copy /etc/hostid to the initramfs if it exists; and use zgenhostid when it doesn't.

ahesford · 2020-11-11T14:09:03Z

I think the change to zgenhostid is reasonable because, when run on a live system, it allows the user to run zgenhostid $(hostid) and write a valid /etc/hostid even on musl systems where hostid prints zeros. My understanding is that ZFS (or SPL) looks for spl_hostid on the kernel command line first and, if that is not found, falls back to directly reading /etc/hostid rather than calling gethostid(3). (If there is no /etc/hostid, ZFS will just assume a zero host ID.)

There is still an outstanding issue in the dracut module-setup.sh. On musl, indiscrimininately running zgenhostid $(hostid) in dracut will always produce the wrong behavior.

On musl, assuming the user doesn't force spl_hostid, before the proposed change:

No /etc/hostid => ZFS on the running system assumes a zero host ID, zgenhostid during dracut setup errors out and does not write /etc/hostid to the initramfs, so ZFS in the initramfs will assume a zero host ID. System boots, but dracut is noisy.
With /etc/hostid => ZFS on the running system assumes stored host ID, zgenhostid during dracut setup errors out and does not write /etc/hostid to the initramfs, so ZFS in the initramfs will assume a zero host ID. System will not boot, and dracut is noisy.

After the proposed change:

No /etc/hostid => ZFS on the running system assumes a zero host ID, zgenhostid during dracut setup generates a random /etc/hostid in the initramfs, so ZFS in the initramfs will assume a different, non-zero host ID. System will not boot.
With /etc/hostid => ZFS on the running system assumes stored host ID, zgenhostid during dracut setup generates a random /etc/hostid in the initramfs, so ZFS in the initramfs will assume a different host ID. System will not boot.

The right thing to do in the dracut module is

if [ -f "${dracutsysrootdir}/etc/hostid" ]; then
    cp "${dracutsysrootdir}/etc/hostid" "${initdir}/etc/hostid"
else
    HOSTID="$(hostid)"
    if [ "${HOSTID}" != "00000000" ]; then
        zgenhostid -o "${initdir}/etc/hostid" "${HOSTID}"
    fi
fi

With glibc, this should always result in the initramfs containing an /etc/hostid that matches that of the running system. On musl, this ensures /etc/hostid matches if there is one, and otherwise omits the file from the initramfs so the fall-through case of zero host ID will consistently happen in the initramfs and the running system.

EDIT: Use $dracutsysrootdir in example to respect the possibility of a dracut --sysroot (even though this doesn't actually work in practice).

gyakovlev · 2020-11-11T18:08:52Z

documentation mentions just 0, so maybe docs should mention that user needs to pass 00000000 or 0x00000000 explicitly?
Sure, previous sentence mentions length requirements, but providing just 0 as example value may be a bit confusing.

The other values are shown as 1 and 2^32-1, so I thought it would be best to stick with the same style. Not using 0 feels inconsistent imho, but I can change it.

maybe you are right. let's leave as is.

behlendorf · 2020-11-11T18:13:12Z

@ahesford thanks for the clear explanation. Then it sounds like we need to include your proposed dracut change in this PR to avoid breaking the "No /etc/hostid" case, which while noisy, does work. @ahesford @ericonr what do you think about updating this PR accordingly.

ahesford · 2020-11-11T18:30:37Z

As @ericonr is the musl user here, I'll defer to him to make sure my string comparison is sane, tweak anything necessary and incorporate here.

ericonr · 2020-11-11T18:35:50Z

Sounds good, I will update the PR ASAP.

ahesford · 2020-11-11T18:51:57Z

FYI, it looks like the proper way to cp "${dracutsysrootdir}/etc/hostid" "${initdir}/etc/hostid" in module-setup.sh is actually inst /etc/hostid.

ericonr · 2020-11-13T03:07:25Z

PR updated with @ahesford's suggestions.

gyakovlev · 2020-11-13T03:27:59Z

contrib/dracut/90zfs/module-setup.sh.in

+		inst @sysconfdir@/hostid
+		type mark_hostonly >/dev/null 2>&1 && mark_hostonly @sysconfdir@/hostid
+	elif HOSTID="$(hostid 2>/dev/null)" && [ "${HOSTID}" != "00000000" ]; then
+		zgenhostid -o "${initdir}@sysconfdir@/hostid" "${hostid}"


I think ${hostid} should be ${HOSTID} here.

also I think it'd be better to check for zeroes first and resort to copying only if that condition is false and hostid file exists, so behaviour for non-musl systems does not change, besides marking hostonly.

consider alternative code (also added some extra quoting and changed -e to -f test )

HOSTID="$(hostid)" if [ "${HOSTID}" != "00000000" ]; then zgenhostid -o "${initdir}/@sysconfdir@/hostid" "${HOSTID}" type mark_hostonly >/dev/null 2>&1 && mark_hostonly @sysconfdir@/hostid else # probably musl stub gethostid() returned 0 if [ -f "@sysconfdir@/hostid" ]; then inst "@sysconfdir@/hostid" type mark_hostonly >/dev/null 2>&1 && mark_hostonly "@sysconfdir@/hostid" fi fi

if file exists, hostid should already return it's value, but we benefit from zgenhostid's error checking and not blindly copying the file.

You're right that ${hostid} should be ${HOSTID}. I also think that moving from -e to -f in the test is a good choice, although I opted for -e because the immediately preceding tests also use -e.

I think we should reduce the dependency on hostid, not elevate it. ZFS does not use the same mechanism for defining the hostid as the hostid(1) executable; it favors spl_hostid on the kernel command line and falls back to directly reading /etc/hostid (unless spl is configured to look elsewhere with another command-line parameter, but if that happens, this whole block is broken anyway). If /etc/hostid is directly a source of truth for ZFS, let's copy that truth without relying on an intermediary.

From a functional standpoint, the difference only matters if there exists a possibility that hostid(1) prints something other than the contents of /etc/hostid. I don't know whether this possibility exists.

If the hostid check is elevated, I still advocate the construct

if HOSTID="$(hostid 2>/dev/null)" && [ "${HOSTID}" != "00000000" ]

because it's probably a good idea to consider any stdout from a hostid that returns an error code as invalid.

I think we should reduce the dependency on hostid, not elevate it.

I strongly agree with this point: the dracut module uses hostid(1) as if its behavior were guaranteed to be the same as what the kernel module (which reads spl_hostid from the kernel command line or the /etc/hostid file) and the ZFS user space tools (which read /etc/hostid directly as well) do. hostid(1) is an interface to libc's gethostid(3), which, as a non-standard function, can be implemented in all sorts of ways. Moving to treating the hostid as close to what the other parts of ZFS do sounds to me like the best way forward.

From a functional standpoint, the difference only matters if there exists a possibility that hostid(1) prints something other than the contents of /etc/hostid. I don't know whether this possibility exists.

This possibility does exist on glibc, where, if /etc/hostid can't be opened, gethostid(3) twiddles the current IP address into an uint32_t and returns that. This is a value that is not at all guaranteed to stay the same, and is miles away from what ZFS treats as the system's hostid.

Everybody happy with the updated dracut change?

The current state reflects my intended fix.

yeah it's ok.
in gentoo we have another initrd generator which supports zfs (genkernel) and it copies hostid file just fine, instead of generating it.

On systems with musl libc, hostid(1) always prints "00000000", which will cause improper behavior when the 90zfs module is configured in a dracut initramfs. Work around this by copying the host /etc/hostid if the file exists, and otherwise only write /etc/hostid if hostid(1) returns something meaningful. This avoids zgenhostid creating a random /etc/hostid for the initramfs, which could lead to errors when trying to import the pool if spl_hostid isn't defined in the kernel command line. Furthermore, tag the /etc/hostid file as hostonly, since it is system specific and shouldn't be taken into account when trying to use an initramfs generated in one system to boot into a different system. Co-authored-by: Andrew J. Hesford <[email protected]> Signed-off-by: Érico Rolim <[email protected]>

On systems with musl libc, hostid(1) always prints "00000000", which will cause improper behavior when the 90zfs module is configured in a dracut initramfs. Work around this by copying the host /etc/hostid if the file exists, and otherwise only write /etc/hostid if hostid(1) returns something meaningful. This avoids zgenhostid creating a random /etc/hostid for the initramfs, which could lead to errors when trying to import the pool if spl_hostid isn't defined in the kernel command line. Furthermore, tag the /etc/hostid file as hostonly, since it is system specific and shouldn't be taken into account when trying to use an initramfs generated in one system to boot into a different system. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Georgy Yakovlev <[email protected]> Co-authored-by: Andrew J. Hesford <[email protected]> Signed-off-by: Érico Rolim <[email protected]> Closes #11174 Closes #11189

A common usage pattern for zgenhostid, including in the ZFS dracut module, is running it as: zgenhostid $(hostid) However, zgenhostid only accepted hostid arguments greater than 0, which meant that, when the output of hostid(1) was "00000000", zgenhostid would error out, even though 0 is a possible return value for the gethostid(3) function used by hostid(1): - On current musl libc, gethostid(3) is a stub that always returns 0. - On glibc, gethostid(3) will return 0 if /etc/hostid exists but is smaller than 4 bytes. In these cases, it makes more sense for zgenhostid to treat a value of 0 as other parts of the zfs codebase do, meaning that a hostid value couldn't be determined; therefore, it should attempt to generate a random value to write into /etc/hostid. The manpage and usage output have been updated to reflect this. Whitespace has also been fixed in the usage output. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Georgy Yakovlev <[email protected]> Reviewed-by: Andrew J. Hesford <[email protected]> Signed-off-by: Érico Rolim <[email protected]> Closes #11174 Closes #11189

On systems with musl libc, hostid(1) always prints "00000000", which will cause improper behavior when the 90zfs module is configured in a dracut initramfs. Work around this by copying the host /etc/hostid if the file exists, and otherwise only write /etc/hostid if hostid(1) returns something meaningful. This avoids zgenhostid creating a random /etc/hostid for the initramfs, which could lead to errors when trying to import the pool if spl_hostid isn't defined in the kernel command line. Furthermore, tag the /etc/hostid file as hostonly, since it is system specific and shouldn't be taken into account when trying to use an initramfs generated in one system to boot into a different system. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Georgy Yakovlev <[email protected]> Co-authored-by: Andrew J. Hesford <[email protected]> Signed-off-by: Érico Rolim <[email protected]> Closes #11174 Closes #11189

A common usage pattern for zgenhostid, including in the ZFS dracut module, is running it as: zgenhostid $(hostid) However, zgenhostid only accepted hostid arguments greater than 0, which meant that, when the output of hostid(1) was "00000000", zgenhostid would error out, even though 0 is a possible return value for the gethostid(3) function used by hostid(1): - On current musl libc, gethostid(3) is a stub that always returns 0. - On glibc, gethostid(3) will return 0 if /etc/hostid exists but is smaller than 4 bytes. In these cases, it makes more sense for zgenhostid to treat a value of 0 as other parts of the zfs codebase do, meaning that a hostid value couldn't be determined; therefore, it should attempt to generate a random value to write into /etc/hostid. The manpage and usage output have been updated to reflect this. Whitespace has also been fixed in the usage output. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Georgy Yakovlev <[email protected]> Reviewed-by: Andrew J. Hesford <[email protected]> Signed-off-by: Érico Rolim <[email protected]> Closes openzfs#11174 Closes openzfs#11189

On systems with musl libc, hostid(1) always prints "00000000", which will cause improper behavior when the 90zfs module is configured in a dracut initramfs. Work around this by copying the host /etc/hostid if the file exists, and otherwise only write /etc/hostid if hostid(1) returns something meaningful. This avoids zgenhostid creating a random /etc/hostid for the initramfs, which could lead to errors when trying to import the pool if spl_hostid isn't defined in the kernel command line. Furthermore, tag the /etc/hostid file as hostonly, since it is system specific and shouldn't be taken into account when trying to use an initramfs generated in one system to boot into a different system. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Georgy Yakovlev <[email protected]> Co-authored-by: Andrew J. Hesford <[email protected]> Signed-off-by: Érico Rolim <[email protected]> Closes openzfs#11174 Closes openzfs#11189

A common usage pattern for zgenhostid, including in the ZFS dracut module, is running it as: zgenhostid $(hostid) However, zgenhostid only accepted hostid arguments greater than 0, which meant that, when the output of hostid(1) was "00000000", zgenhostid would error out, even though 0 is a possible return value for the gethostid(3) function used by hostid(1): - On current musl libc, gethostid(3) is a stub that always returns 0. - On glibc, gethostid(3) will return 0 if /etc/hostid exists but is smaller than 4 bytes. In these cases, it makes more sense for zgenhostid to treat a value of 0 as other parts of the zfs codebase do, meaning that a hostid value couldn't be determined; therefore, it should attempt to generate a random value to write into /etc/hostid. The manpage and usage output have been updated to reflect this. Whitespace has also been fixed in the usage output. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Georgy Yakovlev <[email protected]> Reviewed-by: Andrew J. Hesford <[email protected]> Signed-off-by: Érico Rolim <[email protected]> Closes openzfs#11174 Closes openzfs#11189

On systems with musl libc, hostid(1) always prints "00000000", which will cause improper behavior when the 90zfs module is configured in a dracut initramfs. Work around this by copying the host /etc/hostid if the file exists, and otherwise only write /etc/hostid if hostid(1) returns something meaningful. This avoids zgenhostid creating a random /etc/hostid for the initramfs, which could lead to errors when trying to import the pool if spl_hostid isn't defined in the kernel command line. Furthermore, tag the /etc/hostid file as hostonly, since it is system specific and shouldn't be taken into account when trying to use an initramfs generated in one system to boot into a different system. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Georgy Yakovlev <[email protected]> Co-authored-by: Andrew J. Hesford <[email protected]> Signed-off-by: Érico Rolim <[email protected]> Closes openzfs#11174 Closes openzfs#11189

behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 10, 2020

ericonr force-pushed the hostid branch from 65172cc to c671495 Compare November 11, 2020 06:00

gyakovlev approved these changes Nov 11, 2020

View reviewed changes

behlendorf approved these changes Nov 11, 2020

View reviewed changes

ahesford mentioned this pull request Nov 11, 2020

Respect ZFS hostid behavior on musl zbm-dev/zfsbootmenu#97

Closed

gyakovlev reviewed Nov 13, 2020

View reviewed changes

ericonr force-pushed the hostid branch from 2491bea to 2fd13d5 Compare November 13, 2020 04:24

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 14, 2020

behlendorf closed this in 9c4b6db Nov 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zgenhostid: accept hostid arguments equal to zero. #11189

zgenhostid: accept hostid arguments equal to zero. #11189

ericonr commented Nov 10, 2020

behlendorf commented Nov 10, 2020

ericonr commented Nov 11, 2020

gyakovlev left a comment •

edited

Loading

ericonr commented Nov 11, 2020

ericonr commented Nov 11, 2020

ahesford commented Nov 11, 2020 •

edited

Loading

gyakovlev commented Nov 11, 2020

behlendorf commented Nov 11, 2020

ahesford commented Nov 11, 2020

ericonr commented Nov 11, 2020

ahesford commented Nov 11, 2020

ericonr commented Nov 13, 2020

gyakovlev Nov 13, 2020 •

edited

Loading

gyakovlev Nov 13, 2020 •

edited

Loading

ahesford Nov 13, 2020

ericonr Nov 13, 2020

behlendorf Nov 14, 2020

ahesford Nov 14, 2020

gyakovlev Nov 14, 2020

zgenhostid: accept hostid arguments equal to zero. #11189

zgenhostid: accept hostid arguments equal to zero. #11189

Conversation

ericonr commented Nov 10, 2020

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

behlendorf commented Nov 10, 2020

ericonr commented Nov 11, 2020

gyakovlev left a comment • edited Loading

Choose a reason for hiding this comment

ericonr commented Nov 11, 2020

ericonr commented Nov 11, 2020

ahesford commented Nov 11, 2020 • edited Loading

gyakovlev commented Nov 11, 2020

behlendorf commented Nov 11, 2020

ahesford commented Nov 11, 2020

ericonr commented Nov 11, 2020

ahesford commented Nov 11, 2020

ericonr commented Nov 13, 2020

gyakovlev Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

gyakovlev Nov 13, 2020 • edited Loading

Choose a reason for hiding this comment

ahesford Nov 13, 2020

Choose a reason for hiding this comment

ericonr Nov 13, 2020

Choose a reason for hiding this comment

behlendorf Nov 14, 2020

Choose a reason for hiding this comment

ahesford Nov 14, 2020

Choose a reason for hiding this comment

gyakovlev Nov 14, 2020

Choose a reason for hiding this comment

gyakovlev left a comment •

edited

Loading

ahesford commented Nov 11, 2020 •

edited

Loading

gyakovlev Nov 13, 2020 •

edited

Loading

gyakovlev Nov 13, 2020 •

edited

Loading