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

login
register
mail settings
Submitter Adhemerval Zanella Netto
Date Nov. 23, 2017, 4:38 p.m.
Message ID <1511455130-19179-1-git-send-email-adhemerval.zanella@linaro.org>
Download mbox | patch
Permalink /patch/24476/
State New
Headers show

Comments

Adhemerval Zanella Netto - Nov. 23, 2017, 4:38 p.m.
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(-)
Florian Weimer - Nov. 23, 2017, 7:08 p.m.
* 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 Netto - Nov. 23, 2017, 9:36 p.m.
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.
* 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 Netto - Nov. 24, 2017, 10:35 a.m.
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.

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;
 }