[dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[David Marchand]

This is something we encountered while working in an OpenShift
environment with SELinux enabled.
In this environment, a DPDK application could create/write to hugepage
files but removing them was refused.
This resulted in dirty files being reused when starting a new DPDK
application and triggered random crashes / erratic behavior.

Getting a SELinux setup can be a challenge, and even more if you add
containers to the picture :-).
So here is a reproducer for the interested testers:

  # cat >wrap.c <<EOF
  #define _GNU_SOURCE
  #include <dlfcn.h>
  #include <errno.h>
  #include <stdio.h>
  #include <string.h>
  #include <sys/stat.h>
  #include <sys/types.h>
  #include <unistd.h>

  int unlink(const char *pathname)
  {
  	static int (*orig)(const char *pathname) = NULL;
  	struct stat st;

  	if (orig == NULL)
  		orig = dlsym(RTLD_NEXT, "unlink");
  	if (strstr(pathname, "rtemap_") != NULL &&
			stat(pathname, &st) == 0) {
  		fprintf(stderr, "### refused unlink for %s\n",
  			pathname);
  		errno = EACCES;
  		return -1;
  	}
  	fprintf(stderr, "### called unlink for %s\n", pathname);
  	return orig(pathname);
  }

  int unlinkat(int dirfd, const char *pathname, int flags)
  {
  	static int (*orig)(int dirfd, const char *pathname, int flags) =
  		NULL;
  	struct stat st;

  	if (orig == NULL)
  		orig = dlsym(RTLD_NEXT, "unlinkat");
  	if (strstr(pathname, "rtemap_") != NULL &&
  			fstatat(dirfd, pathname, &st, flags) == 0) {
  		fprintf(stderr, "### refused unlinkat for %s\n",
  			pathname);
  		errno = EACCES;
  		return -1;
  	}
  	fprintf(stderr, "### called unlinkat for %s\n", pathname);
  	return orig(dirfd, pathname, flags);
  }
  EOF

  # gcc -fPIC -shared  -o libwrap.so wrap.c -ldl
  # \rm /dev/hugepages/rtemap*

  # # First run is fine
  # LD_PRELOAD=libwrap.so dpdk-testpmd -w 0000:01:00.0 -- -i
  [...]
  Configuring Port 0 (socket 0)
  Port 0: 24:6E:96:3C:52:D8
  Checking link statuses...
  Done
  testpmd>

  # # Second run we have dirty memory
  # LD_PRELOAD=libwrap.so dpdk-testpmd -w 0000:01:00.0 -- -i
  [...]
  ### refused unlinkat for rtemap_0
  [...]
  Port 0 is now not stopped
  Please stop the ports first
  Done
  testpmd>

Removing hugepage files is done in multiple places and the memory
allocation code is complex.
This fix tries to do the minimum and avoids touching other paths.

If trying to remove the hugepage file before allocating a page fails,
the error is reported to the caller and the user will see a memory
allocation error log.

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/librte_eal/linux/eal_memalloc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/lib/librte_eal/linux/eal_memalloc.c b/lib/librte_eal/linux/eal_memalloc.c
index db60e79975..40a5c4aa1d 100644
--- a/lib/librte_eal/linux/eal_memalloc.c
+++ b/lib/librte_eal/linux/eal_memalloc.c
@@ -329,6 +329,21 @@ get_seg_fd(char *path, int buflen, struct hugepage_info *hi,
 		fd = fd_list[list_idx].fds[seg_idx];
 
 		if (fd < 0) {
+			/* A primary process is the only one creating these
+			 * files. If there is a leftover that was not cleaned
+			 * by clear_hugedir(), we must *now* make sure to drop
+			 * the file or we will remap old stuff while the rest
+			 * of the code is built on the assumption that a new
+			 * page is clean.
+			 */
+			if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
+					unlink(path) == -1 &&
+					errno != ENOENT) {
+				RTE_LOG(DEBUG, EAL, "%s(): could not remove '%s': %s\n",
+					__func__, path, strerror(errno));
+				return -1;
+			}
+
 			fd = open(path, O_CREAT | O_RDWR, 0600);
 			if (fd < 0) {
 				RTE_LOG(DEBUG, EAL, "%s(): open failed: %s\n",
-- 
2.23.0

Re: [dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[Burakov, Anatoly]

On 10-Sep-20 5:24 PM, David Marchand wrote:
> This is something we encountered while working in an OpenShift
> environment with SELinux enabled.
> In this environment, a DPDK application could create/write to hugepage
> files but removing them was refused.
> This resulted in dirty files being reused when starting a new DPDK
> application and triggered random crashes / erratic behavior.
> 
> Getting a SELinux setup can be a challenge, and even more if you add
> containers to the picture :-).
> So here is a reproducer for the interested testers:
> 
>    # cat >wrap.c <<EOF
>    #define _GNU_SOURCE
>    #include <dlfcn.h>
>    #include <errno.h>
>    #include <stdio.h>
>    #include <string.h>
>    #include <sys/stat.h>
>    #include <sys/types.h>
>    #include <unistd.h>
> 
>    int unlink(const char *pathname)
>    {
>    	static int (*orig)(const char *pathname) = NULL;
>    	struct stat st;
> 
>    	if (orig == NULL)
>    		orig = dlsym(RTLD_NEXT, "unlink");
>    	if (strstr(pathname, "rtemap_") != NULL &&
> 			stat(pathname, &st) == 0) {
>    		fprintf(stderr, "### refused unlink for %s\n",
>    			pathname);
>    		errno = EACCES;
>    		return -1;
>    	}
>    	fprintf(stderr, "### called unlink for %s\n", pathname);
>    	return orig(pathname);
>    }
> 
>    int unlinkat(int dirfd, const char *pathname, int flags)
>    {
>    	static int (*orig)(int dirfd, const char *pathname, int flags) =
>    		NULL;
>    	struct stat st;
> 
>    	if (orig == NULL)
>    		orig = dlsym(RTLD_NEXT, "unlinkat");
>    	if (strstr(pathname, "rtemap_") != NULL &&
>    			fstatat(dirfd, pathname, &st, flags) == 0) {
>    		fprintf(stderr, "### refused unlinkat for %s\n",
>    			pathname);
>    		errno = EACCES;
>    		return -1;
>    	}
>    	fprintf(stderr, "### called unlinkat for %s\n", pathname);
>    	return orig(dirfd, pathname, flags);
>    }
>    EOF
> 
>    # gcc -fPIC -shared  -o libwrap.so wrap.c -ldl
>    # \rm /dev/hugepages/rtemap*
> 
>    # # First run is fine
>    # LD_PRELOAD=libwrap.so dpdk-testpmd -w 0000:01:00.0 -- -i
>    [...]
>    Configuring Port 0 (socket 0)
>    Port 0: 24:6E:96:3C:52:D8
>    Checking link statuses...
>    Done
>    testpmd>
> 
>    # # Second run we have dirty memory
>    # LD_PRELOAD=libwrap.so dpdk-testpmd -w 0000:01:00.0 -- -i
>    [...]
>    ### refused unlinkat for rtemap_0
>    [...]
>    Port 0 is now not stopped
>    Please stop the ports first
>    Done
>    testpmd>
> 
> Removing hugepage files is done in multiple places and the memory
> allocation code is complex.
> This fix tries to do the minimum and avoids touching other paths.
> 
> If trying to remove the hugepage file before allocating a page fails,
> the error is reported to the caller and the user will see a memory
> allocation error log.
> 
> Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

I believe only the primary will try to allocate new pages, but this only 
covers the page-per-file scenario. There's also legacy mem option (which 
would attempt to remove files prior to creating new ones - not sure if 
it's refused in that case), and single file segments option (which will 
mostly fallocate holes rather than delete files, but may still attempt 
to delete files - by the way, how does fallocate work with SELinux?).

So, for the contents of the patch in question, it's good, but *might* be 
incomplete for the above reasons. I can't test this right this moment so 
i'll leave this up to you :P

-- 
Thanks,
Anatoly

Re: [dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[David Marchand]

On Fri, Sep 11, 2020 at 2:46 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
> > Removing hugepage files is done in multiple places and the memory
> > allocation code is complex.
> > This fix tries to do the minimum and avoids touching other paths.
> >
> > If trying to remove the hugepage file before allocating a page fails,
> > the error is reported to the caller and the user will see a memory
> > allocation error log.
> >
> > Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> I believe only the primary will try to allocate new pages, but this only
> covers the page-per-file scenario. There's also legacy mem option (which
> would attempt to remove files prior to creating new ones - not sure if
> it's refused in that case), and single file segments option (which will
> mostly fallocate holes rather than delete files, but may still attempt
> to delete files - by the way, how does fallocate work with SELinux?).

This issue should only concern part of the memory allocator that deals
with "named files" backed hugepages.
I would focus on the code relying on eal_get_hugefile_path() and I
agree I might have missed the legacy-mem part.

For anonymous hugepages this should not matter.


-- 
David Marchand

Re: [dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[Burakov, Anatoly]

On 17-Sep-20 2:47 PM, David Marchand wrote:
> On Fri, Sep 11, 2020 at 2:46 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>> Removing hugepage files is done in multiple places and the memory
>>> allocation code is complex.
>>> This fix tries to do the minimum and avoids touching other paths.
>>>
>>> If trying to remove the hugepage file before allocating a page fails,
>>> the error is reported to the caller and the user will see a memory
>>> allocation error log.
>>>
>>> Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>
>> I believe only the primary will try to allocate new pages, but this only
>> covers the page-per-file scenario. There's also legacy mem option (which
>> would attempt to remove files prior to creating new ones - not sure if
>> it's refused in that case), and single file segments option (which will
>> mostly fallocate holes rather than delete files, but may still attempt
>> to delete files - by the way, how does fallocate work with SELinux?).
> 
> This issue should only concern part of the memory allocator that deals
> with "named files" backed hugepages.
> I would focus on the code relying on eal_get_hugefile_path() and I
> agree I might have missed the legacy-mem part.
> 
> For anonymous hugepages this should not matter.
> 
> 

Anonymous hugepages shouldn't matter, yes, but single-file segments mode 
does fallocate() and remove - you have the remove part covered, but i'm 
just curious if fallocate() would also cause any issues with SELinux.

-- 
Thanks,
Anatoly

Re: [dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[Aaron Conole]

"Burakov, Anatoly" <anatoly.burakov@intel.com> writes:

> On 17-Sep-20 2:47 PM, David Marchand wrote:
>> On Fri, Sep 11, 2020 at 2:46 PM Burakov, Anatoly
>> <anatoly.burakov@intel.com> wrote:
>>>> Removing hugepage files is done in multiple places and the memory
>>>> allocation code is complex.
>>>> This fix tries to do the minimum and avoids touching other paths.
>>>>
>>>> If trying to remove the hugepage file before allocating a page fails,
>>>> the error is reported to the caller and the user will see a memory
>>>> allocation error log.
>>>>
>>>> Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>>> ---
>>>
>>> I believe only the primary will try to allocate new pages, but this only
>>> covers the page-per-file scenario. There's also legacy mem option (which
>>> would attempt to remove files prior to creating new ones - not sure if
>>> it's refused in that case), and single file segments option (which will
>>> mostly fallocate holes rather than delete files, but may still attempt
>>> to delete files - by the way, how does fallocate work with SELinux?).
>>
>> This issue should only concern part of the memory allocator that deals
>> with "named files" backed hugepages.
>> I would focus on the code relying on eal_get_hugefile_path() and I
>> agree I might have missed the legacy-mem part.
>>
>> For anonymous hugepages this should not matter.
>>
>>
>
> Anonymous hugepages shouldn't matter, yes, but single-file segments
> mode does fallocate() and remove - you have the remove part covered,
> but i'm just curious if fallocate() would also cause any issues with
> SELinux.

I think it might depend on the policy, file types, and labels allowed
for the process.  I did a little bit of digging but didn't find anything
that specifically targets that call.  I guess it may be something like:

  allow dom_t obj_t:file { getattr setattr appent ioctl lock open read
                           write };

BUT that's just a guess without any real testing.

Re: [dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[David Marchand]

On Thu, Sep 17, 2020 at 4:17 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
> Anonymous hugepages shouldn't matter, yes, but single-file segments mode
> does fallocate() and remove - you have the remove part covered, but i'm
> just curious if fallocate() would also cause any issues with SELinux.

I found no hook in the kernel for fallocate + selinux...
Looked into fallocate itself and it ends up validating lsm write
access on the file.

I don't have the full setup atm but since I could truncate and write
to it, I'd say we are good.


-- 
David Marchand

Re: [dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[David Marchand]

On Thu, Sep 17, 2020 at 4:47 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Thu, Sep 17, 2020 at 4:17 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
> > Anonymous hugepages shouldn't matter, yes, but single-file segments mode
> > does fallocate() and remove - you have the remove part covered, but i'm
> > just curious if fallocate() would also cause any issues with SELinux.
>
> I found no hook in the kernel for fallocate + selinux...
> Looked into fallocate itself and it ends up validating lsm write
> access on the file.
>
> I don't have the full setup atm but since I could truncate and write
> to it, I'd say we are good.

I could not gain access to the same setup again.

FWIW, I tried with my reproducer:
- no issue with --in-memory option (with or without patch)

- error correctly detected (with this patch) in normal mode after restarting:

# \rm /dev/hugepages/rtemap_*
# LD_PRELOAD=libwrap.so dpdk-testpmd -w 0000:01:00.0 -- -i
[... working fine ...]
# LD_PRELOAD=libwrap.so dpdk-testpmd -w 0000:01:00.0 -- -i
EAL: Detected 28 lcore(s)
EAL: Detected 1 NUMA nodes
### called unlink for /var/run/dpdk/rte/mp_socket
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
### refused unlinkat for rtemap_0
EAL: Probing VFIO support...
EAL: VFIO support initialized
### refused unlink for /dev/hugepages/rtemap_0
EAL: Couldn't get fd on hugepage file
EAL: error allocating rte services array
EAL: FATAL: rte_service_init() failed
EAL: rte_service_init() failed
EAL: Error - exiting with code: 1
  Cause: Cannot init EAL: Exec format error
### called unlink for /var/run/dpdk/rte/mp_socket

- error detected with legacy mode from first try (with or without
patch), since the memory allocator tries to remove unneeded hugepage
files in this mode, and reports failures for this:

# \rm /dev/hugepages/rtemap_*
# LD_PRELOAD=libwrap.so dpdk-testpmd -w 0000:01:00.0 --legacy-mem -m 2048 -- -i
EAL: Detected 28 lcore(s)
EAL: Detected 1 NUMA nodes
### called unlink for /var/run/dpdk/rte/mp_socket
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
EAL: Probing VFIO support...
EAL: VFIO support initialized
### refused unlink for /dev/hugepages/rtemap_2
EAL: unmap_unneeded_hugepages(): Removing /dev/hugepages/rtemap_2
failed: Permission denied
EAL: Unmapping and locking hugepages failed!
EAL: FATAL: Cannot init memory
EAL: Cannot init memory
EAL: Error - exiting with code: 1
  Cause: Cannot init EAL: Cannot allocate memory
### called unlink for /var/run/dpdk/rte/mp_socket


-- 
David Marchand

Re: [dpdk-stable] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[Burakov, Anatoly]

On 02-Oct-20 10:36 AM, David Marchand wrote:
> On Thu, Sep 17, 2020 at 4:47 PM David Marchand
> <david.marchand@redhat.com> wrote:
>>
>> On Thu, Sep 17, 2020 at 4:17 PM Burakov, Anatoly
>> <anatoly.burakov@intel.com> wrote:
>>> Anonymous hugepages shouldn't matter, yes, but single-file segments mode
>>> does fallocate() and remove - you have the remove part covered, but i'm
>>> just curious if fallocate() would also cause any issues with SELinux.
>>
>> I found no hook in the kernel for fallocate + selinux...
>> Looked into fallocate itself and it ends up validating lsm write
>> access on the file.
>>
>> I don't have the full setup atm but since I could truncate and write
>> to it, I'd say we are good.
> 
> I could not gain access to the same setup again.
> 
> FWIW, I tried with my reproducer:
> - no issue with --in-memory option (with or without patch)
> 
> - error correctly detected (with this patch) in normal mode after restarting:
> 

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal/linux: fix memory allocations in containers+SELinux

[Thomas Monjalon]

02/10/2020 14:12, Burakov, Anatoly:
> On 02-Oct-20 10:36 AM, David Marchand wrote:
> > On Thu, Sep 17, 2020 at 4:47 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> >>
> >> On Thu, Sep 17, 2020 at 4:17 PM Burakov, Anatoly
> >> <anatoly.burakov@intel.com> wrote:
> >>> Anonymous hugepages shouldn't matter, yes, but single-file segments mode
> >>> does fallocate() and remove - you have the remove part covered, but i'm
> >>> just curious if fallocate() would also cause any issues with SELinux.
> >>
> >> I found no hook in the kernel for fallocate + selinux...
> >> Looked into fallocate itself and it ends up validating lsm write
> >> access on the file.
> >>
> >> I don't have the full setup atm but since I could truncate and write
> >> to it, I'd say we are good.
> > 
> > I could not gain access to the same setup again.
> > 
> > FWIW, I tried with my reproducer:
> > - no issue with --in-memory option (with or without patch)
> > 
> > - error correctly detected (with this patch) in normal mode after restarting:
> 
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

Applied, thanks