From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id 21AEA1B85E for ; Thu, 10 May 2018 14:00:32 +0200 (CEST) Received: by mail-wm0-f48.google.com with SMTP id n10-v6so3925034wmc.1 for ; Thu, 10 May 2018 05:00:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netronome-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=PRQThYBWPNQbKTDU8ALWBRqyEWe8/kKOZVdtydjj4GY=; b=yCdkk/2UObJO0i+B+aLvMPdyecJJAtYGtNmtrvl085BI/sNvqbxPVRbg1pw7qbXPBc dUkDc7jV2+93zj3aFW6bHimpJHHaBBboaRmkA6RsQxLAEZxyEE4luADwqARs9c46Q4d8 PITl/IEIwgG/7iIKTjTmbPkzaBfQCaOJRhdZIAcpv8I19cyivYqCPO2ehFwyZuRGkHHj ob18pLLOD1dXcbir/mA1rWaiKPXuUkBFPOoWaJ17L9CVHURxA95+/1iJ2HgLoPwJ7BI0 P8ND5BWEGPmhUtfpzBH15l+HyhSvEj/M/7tV4EtqINFUhRzWiRDrJHmBj4QDUQuq5jBw kytA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=PRQThYBWPNQbKTDU8ALWBRqyEWe8/kKOZVdtydjj4GY=; b=Yt27BCdClwwNwRqTzXpIQkfl1KVEMf66n3b5g5rbWnLHK4oJzWhV2sug52nZDGlmAN yOPRibyMDnKsTrMec8MYSZNH8JRh8zNzLpyxnB1shAB8h/yZWJhqJ5O9TWfah5tvduUm VGptD2qNOYLajbY15dszglzKzhGBsIs7quqkxC2FO4I2SZNRTex4QOaYf9vKSUOOTlaA gMOqn2V0wOriZb/BaxwW/Mcali7Dmy9Ju3PVdoq4lwx09nZxvlqdpgQodn8O+0s2W5uY 8v4bWKXT56BUi2qY3ijDxkP1saAad7RkS2j58X2CUcqraZBansZhU6MS/jPweYmBj6f3 ZSzA== X-Gm-Message-State: ALKqPwe8EMQ1tO0nRJgjd3aBgHWHN8HIec6VfAl9e1lc7PPPK3O9Sh2R Ae7Cwuo735H9d4wLedZBVne6A3VBic3E3ChBfmvyQA== X-Google-Smtp-Source: AB8JxZoDECnHyjpMd8Rq/tGqXPA5ehMm7zPQaepiiYl35AWwyq3dzMpMQuRNY7696Knivak5GAdulGIHlK9kh1cVtoo= X-Received: by 2002:a50:a621:: with SMTP id d30-v6mr1915325edc.173.1525953631926; Thu, 10 May 2018 05:00:31 -0700 (PDT) MIME-Version: 1.0 Received: by 10.80.212.155 with HTTP; Thu, 10 May 2018 05:00:31 -0700 (PDT) In-Reply-To: References: <20180430172040.13506-1-aconole@redhat.com> <20180430172040.13506-3-aconole@redhat.com> From: Alejandro Lucero Date: Thu, 10 May 2018 13:00:31 +0100 Message-ID: To: Aaron Conole Cc: stable@dpdk.org, Ferruh Yigit , Yuanhan Liu , Eelco Chaudron , Pablo Cascon , Kevin Traynor , Adrien Mazarguil Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-stable] [PATCH 2/2] nfp: allow for non-root user X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 May 2018 12:00:32 -0000 On Wed, May 9, 2018 at 8:44 PM, Alejandro Lucero < alejandro.lucero@netronome.com> wrote: > > > On Wed, May 9, 2018 at 6:53 PM, Aaron Conole wrote: > >> Alejandro Lucero writes: >> >> > I have been thinking about this, and there was something that did not >> seem right to me, although I >> > could not explain what exactly. But this was because we have been >> thinking about VFIO and we have >> > forgotten UIO. The point is, the lock is not required with VFIO but it >> is with UIO, and I'm afraid the >> > way we are trying to solve the non-root user problem is not the right >> one. >> >> Okay, makes sense. >> >> > With VFIO the BARs mapping is done through the kernel VFIO driver, so >> once the device is bound to >> > the driver, and someone tries to use that device, the VFIO driver >> ensures there will not be another >> > user trying to access the device. However, with UIO the driver is not >> doing the BAR mapping but it is >> > the app using the sysfs resource files for that device. It could be, >> and in fact it is easy to happen, two >> > DPDK apps using the same device, because with DPDK apps there is no >> awareness of what other >> > DPDK apps are doing. It is in this case where the lock is required, and >> creating the lock file in the >> > user's home directory is not going to help. >> >> Also makes sense. >> >> > I know RH is just interested in using VFIO, but there are other >> potential NFP PMD users who may >> > want to use UIO instead, and the current solution does not offer the >> right protection in that case. >> > Maybe the lock patch could be created based on the driver the device is >> bound to, leaving /var/lock >> > for UIO and the suggested path for the VFIO case. Or even no lock at >> all for the VFIO case. >> >> Well, yes and no. For instance, in the uio case, they will be running >> most likely as the root user (because to run as non-root in the uio case >> would cause other problems). And in that case, $HOME for all >> applications will be root, yes? I think then, it won't matter. >> >> > I think "most likely" is not enough. If RH wants to run OVS as non-root, > don't you think there could be other people with same necessities? and > using UIO because IOMMU is not available? > > I have been doing some tests and it turns out it is not possible to run DPDK apps as non-root user. There are several files that need to be accessed and it does require to give permissions beforehand to that non-root user, which can be done. But it is not possible for non-root users to get the hugepages physical addresses, because the pagemap file at /proc/self/ is just accessible to root and this file is created by the kernel when the process is created. For VFIO, the hugepages physical address is not required by the PMD but just IOVAs, and the VFIO driver will create the IOMMU mappings with those IOVAs and the right physical addresses. Therefore, I think it is safe enough to keep the user home directory for the lock file, which for the UIO case will always be the root directory. > I do agree with no lock for the vfio case would be best. Is it relevant >> for the newer NFP driver? I haven't had a look yet. If so, then I >> think your proposals are fine there. >> >> For the older one, I only know of one patch that needs to go in (which >> fixes the accidentally lost unlink() call). After that, I wouldn't >> expect more changes in this area (since each one risks stability of the >> code base for a driver that has been replaced). >> >> > On Mon, Apr 30, 2018 at 6:20 PM, Aaron Conole >> wrote: >> > >> > Currently, the nfp lock files are taken from the global lock file >> > location, which will work when the user is running as root. However, >> > some distributions and applications (notably ovs 2.8+ on RHEL/Fedora) >> > run as a non-root user. >> > >> > Acked-by: Alejandro Lucero >> > Signed-off-by: Aaron Conole >> > --- >> > drivers/net/nfp/nfp_nfpu.c | 23 ++++++++++++++++++----- >> > 1 file changed, 18 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/net/nfp/nfp_nfpu.c b/drivers/net/nfp/nfp_nfpu.c >> > index 2ed985ff4..ae2e07220 100644 >> > --- a/drivers/net/nfp/nfp_nfpu.c >> > +++ b/drivers/net/nfp/nfp_nfpu.c >> > @@ -18,6 +18,22 @@ >> > #define NFP_CFG_EXP_BAR 7 >> > >> > #define NFP_CFG_EXP_BAR_CFG_BASE 0x30000 >> > +#define NFP_LOCKFILE_PATH_FMT "%s/nfp%d" >> > + >> > +/* get nfp lock file path (/var/lock if root, $HOME otherwise) */ >> > +static void >> > +nspu_get_lockfile_path(char *buffer, int bufsz, nfpu_desc_t *desc) >> > +{ >> > + const char *dir = "/var/lock"; >> > + const char *home_dir = getenv("HOME"); >> > + >> > + if (getuid() != 0 && home_dir != NULL) >> > + dir = home_dir; >> > + >> > + /* use current prefix as file path */ >> > + snprintf(buffer, bufsz, NFP_LOCKFILE_PATH_FMT, dir, >> > + desc->nfp); >> > +} >> > >> > /* There could be other NFP userspace tools using the NSP interface. >> > * Make sure there is no other process using it and locking the >> access for >> > @@ -30,9 +46,7 @@ nspv_aquire_process_lock(nfpu_desc_t *desc) >> > struct flock lock; >> > char lockname[30]; >> > >> > - memset(&lock, 0, sizeof(lock)); >> > - >> > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", >> desc->nfp); >> > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); >> > >> > /* Using S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | >> S_IWOTH */ >> > desc->lock = open(lockname, O_RDWR | O_CREAT, 0666); >> > @@ -106,7 +120,6 @@ nfpu_close(nfpu_desc_t *desc) >> > rte_free(desc->nspu); >> > close(desc->lock); >> > >> > - snprintf(lockname, sizeof(lockname), "/var/lock/nfp%d", >> desc->nfp); >> > - unlink(lockname); >> > + nspu_get_lockfile_path(lockname, sizeof(lockname), desc); >> > return 0; >> > } >> > -- >> > 2.14.3 >> > >