Quantcast

copyin32(9) for i386 and amd64

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

copyin32(9) for i386 and amd64

Mark Kettenis
We can just call copyin(9) since it already is atomic.  But check
whether the userland futex is properly aligned and return EFAULT if it
isn't such that this system call behaves like it does on strict
alignment architectures.

Also, add a prototype to <sys/systm.h> such that we can actually use it.

ok?


Index: sys/systm.h
===================================================================
RCS file: /cvs/src/sys/sys/systm.h,v
retrieving revision 1.129
diff -u -p -r1.129 systm.h
--- sys/systm.h 15 May 2017 12:26:00 -0000 1.129
+++ sys/systm.h 16 May 2017 21:41:59 -0000
@@ -207,6 +207,7 @@ int copyoutstr(const void *, void *, siz
 int copyin(const void *, void *, size_t)
  __attribute__ ((__bounded__(__buffer__,2,3)));
 int copyout(const void *, void *, size_t);
+int copyin32(const uint32_t *, uint32_t *);
 
 void arc4random_buf(void *, size_t)
  __attribute__ ((__bounded__(__buffer__,1,2)));
Index: arch/i386/i386/machdep.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/machdep.c,v
retrieving revision 1.600
diff -u -p -r1.600 machdep.c
--- arch/i386/i386/machdep.c 30 Apr 2017 16:45:45 -0000 1.600
+++ arch/i386/i386/machdep.c 16 May 2017 21:41:59 -0000
@@ -3890,6 +3890,16 @@ splassert_check(int wantipl, const char
 }
 #endif
 
+int
+copyin32(const uint32_t *uaddr, uint32_t *kaddr)
+{
+ if ((vaddr_t)uaddr & 0x3)
+ return EFAULT;
+
+ /* copyin(9) is atomic */
+ return copyin(uaddr, kaddr, sizeof(uint32_t));
+}
+
 /*
  * True if the system has any non-level interrupts which are shared
  * on the same pin.
Index: arch/amd64/amd64/machdep.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/machdep.c,v
retrieving revision 1.228
diff -u -p -r1.228 machdep.c
--- arch/amd64/amd64/machdep.c 30 Apr 2017 16:45:45 -0000 1.228
+++ arch/amd64/amd64/machdep.c 16 May 2017 21:41:59 -0000
@@ -1777,6 +1777,16 @@ splassert_check(int wantipl, const char
 }
 #endif
 
+int
+copyin32(const uint32_t *uaddr, uint32_t *kaddr)
+{
+ if ((vaddr_t)uaddr & 0x3)
+ return EFAULT;
+
+ /* copyin(9) is atomic */
+ return copyin(uaddr, kaddr, sizeof(uint32_t));
+}
+
 void
 getbootinfo(char *bootinfo, int bootinfo_size)
 {

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: copyin32(9) for i386 and amd64

Ted Unangst-6
Mark Kettenis wrote:
> We can just call copyin(9) since it already is atomic.  But check
> whether the userland futex is properly aligned and return EFAULT if it
> isn't such that this system call behaves like it does on strict
> alignment architectures.

hmm. do we want this? i understand the appeal, but due to differing
compilers/etc, some structs that are carefully packed on some platforms may
not be aligned on i386. however, they would be correctly aligned where
required.

are we trying to prevent a problem that doesn't exist?
but not a major objection.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: copyin32(9) for i386 and amd64

Mark Kettenis
> From: "Ted Unangst" <[hidden email]>
> Date: Tue, 16 May 2017 18:27:13 -0400
>
> Mark Kettenis wrote:
> > We can just call copyin(9) since it already is atomic.  But check
> > whether the userland futex is properly aligned and return EFAULT if it
> > isn't such that this system call behaves like it does on strict
> > alignment architectures.
>
> hmm. do we want this? i understand the appeal, but due to differing
> compilers/etc, some structs that are carefully packed on some platforms may
> not be aligned on i386. however, they would be correctly aligned where
> required.

If you want atomicity on amd64/i386, things must not cross a
cache-line boundary.  The alignment check is a bit stronger than that
but simpler to implement and ensures consistency across platforms.

> are we trying to prevent a problem that doesn't exist?

Maybe.  The i386 ABI requires 32-bit integers to be naturally aligned.
It's 64-bit integers where it has a relaxed requirement.  So unless
your structs are __packed the alignment check should never fail.
Expecting atomic access to a memeber of a __packed struct is a bug.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: copyin32(9) for i386 and amd64

Ted Unangst-6
Mark Kettenis wrote:

> > From: "Ted Unangst" <[hidden email]>
> > Date: Tue, 16 May 2017 18:27:13 -0400
> >
> > Mark Kettenis wrote:
> > > We can just call copyin(9) since it already is atomic.  But check
> > > whether the userland futex is properly aligned and return EFAULT if it
> > > isn't such that this system call behaves like it does on strict
> > > alignment architectures.
> >
> > hmm. do we want this? i understand the appeal, but due to differing
> > compilers/etc, some structs that are carefully packed on some platforms may
> > not be aligned on i386. however, they would be correctly aligned where
> > required.
>
> If you want atomicity on amd64/i386, things must not cross a
> cache-line boundary.  The alignment check is a bit stronger than that
> but simpler to implement and ensures consistency across platforms.
>
> > are we trying to prevent a problem that doesn't exist?
>
> Maybe.  The i386 ABI requires 32-bit integers to be naturally aligned.
> It's 64-bit integers where it has a relaxed requirement.  So unless
> your structs are __packed the alignment check should never fail.
> Expecting atomic access to a memeber of a __packed struct is a bug.

Sounds reasonable to me.

Loading...