Re: git: 47a57144af25 - main - cpuset: Byte swap cpuset for compat32 on big endian architectures

Reply: Dmitry Chagin : "Re: git: 47a57144af25 - main - cpuset: Byte swap cpuset for compat32 on big endian architectures"
Go to: [ bottom of page ] [ top of archives ] [ this month ]
From: Justin Hibbits <chmeee_at_has.gonegalt.net>
Date: Thu, 19 May 2022 18:44:21 UTC
 
"Dmitry Chagin" dchagin@freebsd.org – May 19, 2022 12:49 PM
> On Thu, May 19, 2022 at 03:48:42PM +0000, Justin Hibbits wrote:
> > The branch main has been updated by jhibbits:
> > 
> > URL: https://cgit.FreeBSD.org/src/commit/?id=47a57144af25a7bd768b29272d50a36fdf2874ba
> > 
> > commit 47a57144af25a7bd768b29272d50a36fdf2874ba
> > Author: Justin Hibbits <jhibbits@FreeBSD.org>
> > AuthorDate: 2022-05-12 20:38:22 +0000
> > Commit: Justin Hibbits <jhibbits@FreeBSD.org>
> > CommitDate: 2022-05-19 15:49:55 +0000
> > 
> > cpuset: Byte swap cpuset for compat32 on big endian architectures
> > 
> > Summary:
> > BITSET uses long as its basic underlying type, which is dependent on the
> > compile type, meaning on 32-bit builds the basic type is 32 bits, but on
> > 64-bit builds it's 64 bits. On little endian architectures this doesn't
> > matter, because the LSB is always at the low bit, so the words get
> > effectively concatenated moving between 32-bit and 64-bit, but on
> > big-endian architectures it throws a wrench in, as setting bit 0 in
> > 32-bit mode is equivalent to setting bit 32 in 64-bit mode. To
> > demonstrate:
> > 
> > 32-bit mode:
> > 
> > BIT_SET(foo, 0): 0x00000001
> > 
> > 64-bit sees: 0x0000000100000000
> > 
> > cpuset is the only system interface that uses bitsets, so solve this
> > by swapping the integer sub-components at the copyin/copyout points.
> > 
> > Reviewed by: kib
> > MFC after: 3 days
> > Sponsored by: Juniper Networks, Inc.
> > Differential Revision: https://reviews.freebsd.org/D35225
> > ---
> > sys/compat/freebsd32/freebsd32_misc.c | 70 +++++++++++++++++++++++++++++++++--
> > sys/kern/kern_cpuset.c | 32 ++++++++++------
> > sys/sys/cpuset.h | 9 +++++
> > sys/sys/syscallsubr.h | 10 +++--
> > 4 files changed, 101 insertions(+), 20 deletions(-)
> > 
> > diff --git a/sys/compat/freebsd32/freebsd32_misc.c b/sys/compat/freebsd32/freebsd32_misc.c
> > index 25c7a4e17dc7..d4206fdd24aa 100644
> > --- a/sys/compat/freebsd32/freebsd32_misc.c
> > +++ b/sys/compat/freebsd32/freebsd32_misc.c
> > @@ -3310,13 +3310,72 @@ freebsd32_cpuset_getid(struct thread *td,
> > PAIR32TO64(id_t, uap->id), uap->setid));
> > }
> > 
> > +static int
> > +copyin32_set(const void *u, void *k, size_t size)
> > +{
> > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > + int rv;
> > + struct bitset *kb = k;
> > + int *p;
> > +
> > + rv = copyin(u, k, size);
> > + if (rv != 0)
> > + return (rv);
> > +
> > + p = (int *)kb->__bits;
> > + /* Loop through swapping words.
> > + * `size' is in bytes, we need bits. */
> > + for (int i = 0; i < __bitset_words(size * 8); i++) {
> > + int tmp = p[0];
> > + p[0] = p[1];
> > + p[1] = tmp;
> > + p += 2;
> > + }
> > + return (0);
> > +#else
> > + return (copyin(u, k, size));
> > +#endif
> > +}
> > +
> > +static int
> > +copyout32_set(const void *k, void *u, size_t size)
> > +{
> > +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > + const struct bitset *kb = k;
> > + struct bitset *ub = u;
> > + const int *kp = (const int *)kb->__bits;
> > + int *up = (int *)ub->__bits;
> > + int rv;
> > +
> > + for (int i = 0; i < __bitset_words(CPU_SETSIZE); i++) {
> > + /* `size' is in bytes, we need bits. */
> > + for (int i = 0; i < __bitset_words(size * 8); i++) {
> > + rv = suword32(up, kp[1]);
> > + if (rv == 0)
> > + rv = suword32(up + 1, kp[0]);
> > + if (rv != 0)
> > + return (EFAULT);
> > + }
> > + }
> > + return (0);
> > +#else
> > + return (copyout(k, u, size));
> > +#endif
> > +}
> > +
> > +static const struct cpuset_copy_cb cpuset_copy32_cb = {
> > + .copyin = copyin32_set,
> > + .copyout = copyout32_set
> > +};
> > +
> > int
> > freebsd32_cpuset_getaffinity(struct thread *td,
> > struct freebsd32_cpuset_getaffinity_args *uap)
> > {
> > 
> > return (kern_cpuset_getaffinity(td, uap->level, uap->which,
> > - PAIR32TO64(id_t,uap->id), uap->cpusetsize, uap->mask));
> > + PAIR32TO64(id_t,uap->id), uap->cpusetsize, uap->mask,
> > + &cpuset_copy32_cb));
> > }
> > 
> > int
> > @@ -3325,7 +3384,8 @@ freebsd32_cpuset_setaffinity(struct thread *td,
> > {
> > 
> > return (user_cpuset_setaffinity(td, uap->level, uap->which,
> > - PAIR32TO64(id_t,uap->id), uap->cpusetsize, uap->mask));
> > + PAIR32TO64(id_t,uap->id), uap->cpusetsize, uap->mask,
> > + &cpuset_copy32_cb));
> > }
> > 
> > int
> > @@ -3334,7 +3394,8 @@ freebsd32_cpuset_getdomain(struct thread *td,
> > {
> > 
> > return (kern_cpuset_getdomain(td, uap->level, uap->which,
> > - PAIR32TO64(id_t,uap->id), uap->domainsetsize, uap->mask, uap->policy));
> > + PAIR32TO64(id_t,uap->id), uap->domainsetsize, uap->mask, uap->policy,
> > + &cpuset_copy32_cb));
> > }
> > 
> > int
> > @@ -3343,7 +3404,8 @@ freebsd32_cpuset_setdomain(struct thread *td,
> > {
> > 
> > return (kern_cpuset_setdomain(td, uap->level, uap->which,
> > - PAIR32TO64(id_t,uap->id), uap->domainsetsize, uap->mask, uap->policy));
> > + PAIR32TO64(id_t,uap->id), uap->domainsetsize, uap->mask, uap->policy,
> > + &cpuset_copy32_cb));
> > }
> > 
> > int
> > diff --git a/sys/kern/kern_cpuset.c b/sys/kern/kern_cpuset.c
> > index 56d92687a693..e21ec146bb00 100644
> > --- a/sys/kern/kern_cpuset.c
> > +++ b/sys/kern/kern_cpuset.c
> > @@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$");
> > #include <sys/sched.h>
> > #include <sys/smp.h>
> > #include <sys/syscallsubr.h>
> > +#include <sys/sysent.h>
> > #include <sys/capsicum.h>
> > #include <sys/cpuset.h>
> > #include <sys/domainset.h>
> > @@ -1742,6 +1743,11 @@ cpuset_check_capabilities(struct thread *td, cpulevel_t level, cpuwhich_t which,
> > return (0);
> > }
> > 
> > +static const struct cpuset_copy_cb copy_set = {
> > + .copyin = copyin,
> > + .copyout = copyout
> > +};
> > +
> > #ifndef _SYS_SYSPROTO_H_
> > struct cpuset_args {
> > cpusetid_t *setid;
> > @@ -1883,12 +1889,12 @@ sys_cpuset_getaffinity(struct thread *td, struct cpuset_getaffinity_args *uap)
> > {
> > 
> > return (kern_cpuset_getaffinity(td, uap->level, uap->which,
> > - uap->id, uap->cpusetsize, uap->mask));
> > + uap->id, uap->cpusetsize, uap->mask, &copy_set));
> > }
> > 
> > int
> > kern_cpuset_getaffinity(struct thread *td, cpulevel_t level, cpuwhich_t which,
> > - id_t id, size_t cpusetsize, cpuset_t *maskp)
> > + id_t id, size_t cpusetsize, cpuset_t *maskp, const struct cpuset_copy_cb *cb)
> > {
> 
> this broke the build, as Linuxulator also uses kern_cpuset_getaffinity,
> may be we need to move out the userspace related code from
> kern_cpuset_getaffinity to user_cpuset_getaffinity?
> 
 
Sorry, I didn't sanity test a full kernel build, only a kernel-only build.  I see you fixed it with 89737eb, thanks.
 
- Justin