diff mbox

posix: Fix generic p{read,write}v buffer allocation (BZ#22457)

Message ID 1511455130-19179-1-git-send-email-adhemerval.zanella@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Nov. 23, 2017, 4:38 p.m. UTC 
  As described in BZ#22457 an interpose malloc can free an invalid
pointer for fallback preadv implementation.  Fortunately this is
just and issue on microblaze-linux-gnu running kernels older than
3.15.

This patch fixes it by calling mmap/unmap instead of posix_memalign/
free.  A sanity check is address to certify buffer alignment (since
POSIX does not enforces a particular alignment on returned mmap
value, just for MMAP_FIXED input).

Checked on microblaze-linux-gnu check with run-built-tests=no and
by using the sysdeps/posix implementation on x86_64-linux-gnu (just
for sanity test where it shown no regression).

	* include/libc-pointer-arith.h (PTR_IS_ALIGNED): New macro.
	* sysdeps/posix/preadv_common.c (PREADV): Use mmap/munmap instead of
	posix_memalign/free.
	* sysdeps/posix/pwritev_common.c (PWRITEV): Likewise.
---
 ChangeLog                      |  7 +++++++
 include/libc-pointer-arith.h   |  4 ++++
 sysdeps/posix/preadv_common.c  | 10 +++++++---
 sysdeps/posix/pwritev_common.c | 10 +++++++---
 4 files changed, 25 insertions(+), 6 deletions(-)

Comments

Florian Weimer Nov. 23, 2017, 7:08 p.m. UTC | #1 
* Adhemerval Zanella:

> +  size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize));
> +  void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE,
> +		         MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +  if (__glibc_unlikely (buffer == MAP_FAILED)
> +      || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize))))
>      return -1;

I don't think POSIX requires that the length of the mapping is a
multiple of the page size.  You could probably drop the alignment
check, too, because I don't really see any reason why the alignment
requirement would be related to the page size.
Adhemerval Zanella Nov. 23, 2017, 9:36 p.m. UTC | #2 
On 23/11/2017 17:08, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> +  size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize));
>> +  void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE,
>> +		         MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> +  if (__glibc_unlikely (buffer == MAP_FAILED)
>> +      || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize))))
>>      return -1;
> 
> I don't think POSIX requires that the length of the mapping is a
> multiple of the page size.  You could probably drop the alignment
> check, too, because I don't really see any reason why the alignment
> requirement would be related to the page size.
> 

Indeed we do not need to align to page size, however the alignment
requirement was the motivation to actually change its implementation
(check commit message for c79a72aa5cb8357c216a71015c7448a9259c8531).
Florian Weimer Nov. 24, 2017, 3:58 a.m. UTC | #3 
* Adhemerval Zanella:

> On 23/11/2017 17:08, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> +  size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize));
>>> +  void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE,
>>> +		         MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>> +  if (__glibc_unlikely (buffer == MAP_FAILED)
>>> +      || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize))))
>>>      return -1;
>> 
>> I don't think POSIX requires that the length of the mapping is a
>> multiple of the page size.  You could probably drop the alignment
>> check, too, because I don't really see any reason why the alignment
>> requirement would be related to the page size.

> Indeed we do not need to align to page size, however the alignment
> requirement was the motivation to actually change its implementation
> (check commit message for c79a72aa5cb8357c216a71015c7448a9259c8531).

I meant that there is no way to query the alignment required for
O_DIRECT, and that page size alignment might be insufficient.  Some
file systems hard-code a 4K alignment requirement, and microblaze has
at least 4K pages, so we should be good.  But it feels strange to
write a check against the wrong constant.
Adhemerval Zanella Nov. 24, 2017, 10:35 a.m. UTC | #4 
On 24/11/2017 01:58, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 23/11/2017 17:08, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> +  size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize));
>>>> +  void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE,
>>>> +		         MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>>>> +  if (__glibc_unlikely (buffer == MAP_FAILED)
>>>> +      || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize))))
>>>>      return -1;
>>>
>>> I don't think POSIX requires that the length of the mapping is a
>>> multiple of the page size.  You could probably drop the alignment
>>> check, too, because I don't really see any reason why the alignment
>>> requirement would be related to the page size.
> 
>> Indeed we do not need to align to page size, however the alignment
>> requirement was the motivation to actually change its implementation
>> (check commit message for c79a72aa5cb8357c216a71015c7448a9259c8531).
> 
> I meant that there is no way to query the alignment required for
> O_DIRECT, and that page size alignment might be insufficient.  Some
> file systems hard-code a 4K alignment requirement, and microblaze has
> at least 4K pages, so we should be good.  But it feels strange to
> write a check against the wrong constant.
> 

Alright, looks like I am over-enginering it. I will remove the check
as well.
diff mbox

Patch

diff --git a/include/libc-pointer-arith.h b/include/libc-pointer-arith.h
index 715cbc1..c918023 100644
--- a/include/libc-pointer-arith.h
+++ b/include/libc-pointer-arith.h
@@ -57,4 +57,8 @@ 
 #define PTR_ALIGN_UP(base, size) \
   ((__typeof__ (base)) ALIGN_UP ((uintptr_t) (base), (size)))
 
+/* Check if base is aligned to boundary.  */
+#define PTR_IS_ALIGNED(base, boundary) \
+  (((uintptr_t) (base) & ((uintptr_t) ((boundary)) - 1)) == 0)
+
 #endif
diff --git a/sysdeps/posix/preadv_common.c b/sysdeps/posix/preadv_common.c
index 37efdc0..099c37b 100644
--- a/sysdeps/posix/preadv_common.c
+++ b/sysdeps/posix/preadv_common.c
@@ -24,6 +24,7 @@ 
 #include <malloc.h>
 
 #include <ldsodefs.h>
+#include <libc-pointer-arith.h>
 
 /* Read data from file descriptor FD at the given position OFFSET
    without change the file pointer, and put the result in the buffers
@@ -54,8 +55,11 @@  PREADV (int fd, const struct iovec *vector, int count, OFF_T offset)
      but 1. it is system specific (not meant in generic implementation), and
      2. it would make the implementation more complex, and 3. it will require
      another syscall (fcntl).  */
-  void *buffer = NULL;
-  if (__posix_memalign (&buffer, GLRO(dl_pagesize), bytes) != 0)
+  size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize));
+  void *buffer = __mmap (NULL, mmap_size, PROT_READ | PROT_WRITE,
+		         MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (__glibc_unlikely (buffer == MAP_FAILED)
+      || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize))))
     return -1;
 
   ssize_t bytes_read = PREAD (fd, buffer, bytes, offset);
@@ -78,6 +82,6 @@  PREADV (int fd, const struct iovec *vector, int count, OFF_T offset)
     }
 
 end:
-  free (buffer);
+  __munmap (buffer, mmap_size);
   return bytes_read;
 }
diff --git a/sysdeps/posix/pwritev_common.c b/sysdeps/posix/pwritev_common.c
index 0383065..610c54e 100644
--- a/sysdeps/posix/pwritev_common.c
+++ b/sysdeps/posix/pwritev_common.c
@@ -24,6 +24,7 @@ 
 #include <malloc.h>
 
 #include <ldsodefs.h>
+#include <libc-pointer-arith.h>
 
 /* Write data pointed by the buffers described by IOVEC, which is a
    vector of COUNT 'struct iovec's, to file descriptor FD at the given
@@ -54,8 +55,11 @@  PWRITEV (int fd, const struct iovec *vector, int count, OFF_T offset)
      but 1. it is system specific (not meant in generic implementation), and
      2. it would make the implementation more complex, and 3. it will require
      another syscall (fcntl).  */
-  void *buffer = NULL;
-  if (__posix_memalign (&buffer, GLRO(dl_pagesize), bytes) != 0)
+  size_t mmap_size = ALIGN_UP (bytes, GLRO(dl_pagesize));
+  void *buffer = __mmap (NULL, mmap_size, PROT_WRITE,
+		         MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (__glibc_unlikely (buffer == MAP_FAILED)
+      || __glibc_unlikely (!PTR_IS_ALIGNED (buffer, GLRO(dl_pagesize))))
     return -1;
 
   /* Copy the data from BUFFER into the memory specified by VECTOR.  */
@@ -66,7 +70,7 @@  PWRITEV (int fd, const struct iovec *vector, int count, OFF_T offset)
 
   ssize_t ret = PWRITE (fd, buffer, bytes, offset);
 
-  free (buffer);
+  __munmap (buffer, mmap_size);
 
   return ret;
 }