libspl: getexecname() cleanup and gethostid() fixes, /proc/sys/kernel/spl/hostid fix for recent kernels in SPL #11879

nabijaczleweli · 2021-04-11T00:11:16Z

Motivation and Context

See individual commit messages, #11878.

Description

The second patch merges the actual implementations of getexecname() (and slightly cleans up the FreeBSD one, which I haven't tested, but I can't see why it wouldn't be identical (beside maybe the header reduxion going badly)).

The third patch fixes /proc/sys/kernel/spl/hostid on 5.7+ kernels. I haven't actually tested this on any older kernel, but the blame of kernel/sysctl.c#_proc_do_string() says there's no need to worry:

^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  243) 
f88083005ab31 (Kees Cook                      2014-06-06 14:37:17 -0700  244) static int _proc_do_string(char *data, int maxlen, int write,
32927393dc1cc (Christoph Hellwig              2020-04-24 08:43:38 +0200  245) 		char *buffer, size_t *lenp, loff_t *ppos)
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  246) {
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  247) 	size_t len;
32927393dc1cc (Christoph Hellwig              2020-04-24 08:43:38 +0200  248) 	char c, *p;
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  249) 
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  250) 	if (!data || !maxlen || !*lenp) {
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  251) 		*lenp = 0;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  252) 		return 0;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  253) 	}
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  254) 
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  255) 	if (write) {
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  256) 		if (sysctl_writes_strict == SYSCTL_WRITES_STRICT) {
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  257) 			/* Only continue writes not past the end of buffer. */
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  258) 			len = strlen(data);
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  259) 			if (len > maxlen - 1)
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  260) 				len = maxlen - 1;
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  261) 
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  262) 			if (*ppos > len)
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  263) 				return 0;
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  264) 			len = *ppos;
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  265) 		} else {
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  266) 			/* Start writing from beginning of buffer. */
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  267) 			len = 0;
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  268) 		}
f4aacea2f5d1a (Kees Cook                      2014-06-06 14:37:19 -0700  269) 
2ca9bb456ada8 (Kees Cook                      2014-06-06 14:37:18 -0700  270) 		*ppos += *lenp;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  271) 		p = buffer;
2ca9bb456ada8 (Kees Cook                      2014-06-06 14:37:18 -0700  272) 		while ((p - buffer) < *lenp && len < maxlen - 1) {
32927393dc1cc (Christoph Hellwig              2020-04-24 08:43:38 +0200  273) 			c = *(p++);
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  274) 			if (c == 0 || c == '\n')
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  275) 				break;
2ca9bb456ada8 (Kees Cook                      2014-06-06 14:37:18 -0700  276) 			data[len++] = c;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  277) 		}
f88083005ab31 (Kees Cook                      2014-06-06 14:37:17 -0700  278) 		data[len] = 0;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  279) 	} else {
f5dd3d6fadf98 (Sam Vilain                     2006-10-02 02:18:04 -0700  280) 		len = strlen(data);
f5dd3d6fadf98 (Sam Vilain                     2006-10-02 02:18:04 -0700  281) 		if (len > maxlen)
f5dd3d6fadf98 (Sam Vilain                     2006-10-02 02:18:04 -0700  282) 			len = maxlen;
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  283) 
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  284) 		if (*ppos > len) {
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  285) 			*lenp = 0;
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  286) 			return 0;
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  287) 		}
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  288) 
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  289) 		data += *ppos;
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  290) 		len  -= *ppos;
8d06087714b78 (Oleg Nesterov                  2007-02-10 01:46:38 -0800  291) 
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  292) 		if (len > *lenp)
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  293) 			len = *lenp;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  294) 		if (len)
32927393dc1cc (Christoph Hellwig              2020-04-24 08:43:38 +0200  295) 			memcpy(buffer, data, len);
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  296) 		if (len < *lenp) {
32927393dc1cc (Christoph Hellwig              2020-04-24 08:43:38 +0200  297) 			buffer[len] = '\n';
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  298) 			len++;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  299) 		}
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  300) 		*lenp = len;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  301) 		*ppos += len;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  302) 	}
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  303) 	return 0;
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  304) }
^1da177e4c3f4 (Linus Torvalds                 2005-04-16 15:20:36 -0700  305)

The fourth patch (a) fixes get_system_hostid() if it was set via the aforementioned sysctl, (b) makes it mildly faster, considering no-one sets spl_hostid, and (c) simplifies the code a bit.

The fifth patch uses the same limits in dummy tables as in the global ones.

How Has This Been Tested?

Ran the functions, they return the right results (on Linux), the module behaves right on the kernel from #11878.

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) – likely irrelevant, and I'd argue it's a bug-fix, but see the third patch
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. – there's none
I have read the contributing document.
I have added tests to cover my changes. – none apply, I'm pretty sure; maybe some should? but dunno where I'd start with that
I have run the ZFS Test Suite with this change applied. – CI take my hand
All commit messages are properly formatted and contain Signed-off-by.

lib/libspl/os/linux/getexecname.c

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit 32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers to ->proc_handler") ‒ 5.7-rc1 and up The access_ok() check in copy_to_user() in proc_copyout_string() would always fail, so all userspace reads and writes would fail with EINVAL proc_dostring() strips only the final new-line, but simple_strtoul() doesn't actually need a back-trimmed string ‒ writing "012345678 \n" is still allowed, as is "012345678zupsko", &c. This alters what happens when an invalid value is written ‒ previously it'd get set to what-ever simple_strtoul() returned (probably 0, thereby resetting it to default), now it does nothing Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11878

The kernel and user-space must agree, after all Signed-off-by: Ahelenia Ziemiańska <[email protected]>

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

nabijaczleweli · 2021-04-12T08:35:38Z

Rebased

Merge the actual implementations of getexecname() and slightly clean up the FreeBSD one. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #11879

This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit 32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers to ->proc_handler") ‒ 5.7-rc1 and up The access_ok() check in copy_to_user() in proc_copyout_string() would always fail, so all userspace reads and writes would fail with EINVAL proc_dostring() strips only the final new-line, but simple_strtoul() doesn't actually need a back-trimmed string ‒ writing "012345678 \n" is still allowed, as is "012345678zupsko", &c. This alters what happens when an invalid value is written ‒ previously it'd get set to what-ever simple_strtoul() returned (probably 0, thereby resetting it to default), now it does nothing Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #11878 Closes #11879

Fixes get_system_hostid() if it was set via the aforementioned sysctl and simplifies the code a bit. The kernel and user-space must agree, after all. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #11879

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #11879

This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit 32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers to ->proc_handler") ‒ 5.7-rc1 and up The access_ok() check in copy_to_user() in proc_copyout_string() would always fail, so all userspace reads and writes would fail with EINVAL proc_dostring() strips only the final new-line, but simple_strtoul() doesn't actually need a back-trimmed string ‒ writing "012345678 \n" is still allowed, as is "012345678zupsko", &c. This alters what happens when an invalid value is written ‒ previously it'd get set to what-ever simple_strtoul() returned (probably 0, thereby resetting it to default), now it does nothing Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11878 Closes openzfs#11879

Fixes get_system_hostid() if it was set via the aforementioned sysctl and simplifies the code a bit. The kernel and user-space must agree, after all. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Merge the actual implementations of getexecname() and slightly clean up the FreeBSD one. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Merge the actual implementations of getexecname() and slightly clean up the FreeBSD one. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Merge the actual implementations of getexecname() and slightly clean up the FreeBSD one. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Merge the actual implementations of getexecname() and slightly clean up the FreeBSD one. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

This fixes /proc/sys/kernel/spl/hostid on kernels with mainline commit 32927393dc1ccd60fb2bdc05b9e8e88753761469 ("sysctl: pass kernel pointers to ->proc_handler") ‒ 5.7-rc1 and up The access_ok() check in copy_to_user() in proc_copyout_string() would always fail, so all userspace reads and writes would fail with EINVAL proc_dostring() strips only the final new-line, but simple_strtoul() doesn't actually need a back-trimmed string ‒ writing "012345678 \n" is still allowed, as is "012345678zupsko", &c. This alters what happens when an invalid value is written ‒ previously it'd get set to what-ever simple_strtoul() returned (probably 0, thereby resetting it to default), now it does nothing Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11878 Closes openzfs#11879

Fixes get_system_hostid() if it was set via the aforementioned sysctl and simplifies the code a bit. The kernel and user-space must agree, after all. Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

nabijaczleweli force-pushed the smart-practical-logic branch 8 times, most recently from 74a9bdf to 00384ce Compare April 11, 2021 01:43

nabijaczleweli changed the title ~~libspl: getexecname() and gethostid() fixes, /proc/sys/kernel/spl/hostid fix for recent kernels in SPL~~ libspl: getexecname() cleanup and gethostid() fixes, /proc/sys/kernel/spl/hostid fix for recent kernels in SPL Apr 11, 2021

nabijaczleweli force-pushed the smart-practical-logic branch 4 times, most recently from 41fa7c3 to ba070cb Compare April 11, 2021 14:00

behlendorf reviewed Apr 11, 2021

View reviewed changes

lib/libspl/os/linux/getexecname.c Outdated Show resolved Hide resolved

behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 11, 2021

module/zfs/zvol.c: purge unused zvol_volmode_cb_arg

36dc7c3

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

nabijaczleweli force-pushed the smart-practical-logic branch from 413aef9 to 91b3f23 Compare April 12, 2021 08:24

nabijaczleweli added 4 commits April 12, 2021 10:34

libspl: lift common bits of getexecname()

6b251ad

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

linux/libspl: gethostid: read from /proc/sys/kernel/spl/hostid, simplify

53efab3

The kernel and user-space must agree, after all Signed-off-by: Ahelenia Ziemiańska <[email protected]>

linux/spl: proc: use global table_{min,max} values instead of local ones

bdfded7

Signed-off-by: Ahelenia Ziemiańska <[email protected]>

nabijaczleweli force-pushed the smart-practical-logic branch from 91b3f23 to bdfded7 Compare April 12, 2021 08:34

behlendorf approved these changes Apr 15, 2021

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 15, 2021

behlendorf closed this in 375bdb2 Apr 15, 2021

behlendorf pushed a commit that referenced this pull request Apr 15, 2021

linux/spl: proc: use global table_{min,max} values instead of local ones

23b6f17

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #11879

ghost pushed a commit to truenas/zfs that referenced this pull request May 6, 2021

module/zfs/zvol.c: purge unused zvol_volmode_cb_arg

5c8067a

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

ghost pushed a commit to truenas/zfs that referenced this pull request May 6, 2021

module/zfs/zvol.c: purge unused zvol_volmode_cb_arg

147266c

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

ghost pushed a commit to truenas/zfs that referenced this pull request May 6, 2021

module/zfs/zvol.c: purge unused zvol_volmode_cb_arg

769c1aa

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

ghost pushed a commit to truenas/zfs that referenced this pull request May 7, 2021

module/zfs/zvol.c: purge unused zvol_volmode_cb_arg

84b2854

Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11879

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libspl: getexecname() cleanup and gethostid() fixes, /proc/sys/kernel/spl/hostid fix for recent kernels in SPL #11879

libspl: getexecname() cleanup and gethostid() fixes, /proc/sys/kernel/spl/hostid fix for recent kernels in SPL #11879

nabijaczleweli commented Apr 11, 2021 •

edited

Loading

nabijaczleweli commented Apr 12, 2021

libspl: getexecname() cleanup and gethostid() fixes, /proc/sys/kernel/spl/hostid fix for recent kernels in SPL #11879

libspl: getexecname() cleanup and gethostid() fixes, /proc/sys/kernel/spl/hostid fix for recent kernels in SPL #11879

Conversation

nabijaczleweli commented Apr 11, 2021 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

nabijaczleweli commented Apr 12, 2021

nabijaczleweli commented Apr 11, 2021 •

edited

Loading