[v2] net/memif: change socket listener owner uid/gid

Message ID c07a6b1cb6d446ec@cs.arizona.edu (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series [v2] net/memif: change socket listener owner uid/gid |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS

Commit Message

Junxiao Shi Dec. 7, 2022, 2:41 p.m. UTC
  This allows a DPDK application running with root privilege to create a
memif socket listener with non-root owner uid and gid, which can be
connected from client applications running without root privilege.

Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>
---
 doc/guides/nics/memif.rst         |   2 +
 drivers/net/memif/memif_socket.c  |  13 ++-
 drivers/net/memif/rte_eth_memif.c | 129 ++++++++++++++++++++----------
 drivers/net/memif/rte_eth_memif.h |   2 +
 4 files changed, 102 insertions(+), 44 deletions(-)
  

Comments

Ferruh Yigit Dec. 7, 2022, 3:43 p.m. UTC | #1
On 12/7/2022 2:41 PM, Junxiao Shi wrote:
> This allows a DPDK application running with root privilege to create a
> memif socket listener with non-root owner uid and gid, which can be
> connected from client applications running without root privilege.
> 

Do you have an easy way to test unprivileged memif client?

> Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>

<...>

> @@ -1827,47 +1859,58 @@ rte_pmd_memif_probe(struct rte_vdev_device *vdev)
>  	flags |= ETH_MEMIF_FLAG_SOCKET_ABSTRACT;
>  
>  	kvlist = rte_kvargs_parse(rte_vdev_device_args(vdev), valid_arguments);
> +	if (kvlist == NULL) {
> +		MIF_LOG(ERR, "Invalid kvargs key");
> +		ret = -EINVAL;
> +		goto exit;
> +	}

Thanks Junxiao for updating this, but since it is not really related to
this patch, can you please separate it to another patch?
  
Ferruh Yigit Dec. 7, 2022, 4:56 p.m. UTC | #2
On 12/7/2022 3:43 PM, Ferruh Yigit wrote:
> On 12/7/2022 2:41 PM, Junxiao Shi wrote:
>> This allows a DPDK application running with root privilege to create a
>> memif socket listener with non-root owner uid and gid, which can be
>> connected from client applications running without root privilege.
>>
> 
> Do you have an easy way to test unprivileged memif client?
> 
>> Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>
> 
> <...>
> 
>> @@ -1827,47 +1859,58 @@ rte_pmd_memif_probe(struct rte_vdev_device *vdev)
>>  	flags |= ETH_MEMIF_FLAG_SOCKET_ABSTRACT;
>>  
>>  	kvlist = rte_kvargs_parse(rte_vdev_device_args(vdev), valid_arguments);
>> +	if (kvlist == NULL) {
>> +		MIF_LOG(ERR, "Invalid kvargs key");
>> +		ret = -EINVAL;
>> +		goto exit;
>> +	}
> 
> Thanks Junxiao for updating this, but since it is not really related to
> this patch, can you please separate it to another patch?

Also I am not sure what 'rte_kvargs_parse()' returns if there is no
argument provided (that may be the case if all default values are desired).
If API returns NULL for that case, above error log can be wrong, can you
please check.
  
Junxiao Shi Dec. 7, 2022, 5:48 p.m. UTC | #3
Hi Ferruh

> On 12/7/2022 2:41 PM, Junxiao Shi wrote:
> > This allows a DPDK application running with root privilege to create a
> > memif socket listener with non-root owner uid and gid, which can be
> > connected from client applications running without root privilege.
> >
>
> Do you have an easy way to test unprivileged memif client?

This has been tested with NDN-DPDK software.
https://github.com/usnistgov/ndn-dpdk revision
311de078aa4dc3ea28db5f8858e70a1bef7b9ccd

The systemd service is running as root and it uses DPDK with the owner-uid
and owner-gid args.
The ndndpdk-godemo command is running as unprivileged process.
Directory /run/ndn still needs to be created by root.

These commands can perform a full test:

git clone https://github.com/usnistgov/ndn-dpdk.git
cd ndn-dpdk
./docs/ndndpdk-depends.sh --dpdk-patch=26031
corepack pnpm install
make
sudo make install
sudo dpdk-hugepages.py --setup 8G
sudo ndndpdk-ctrl systemd start
jq -n {} | ndndpdk-ctrl activate-forwarder
sudo mkdir -p /run/ndn
ndndpdk-godemo pingserver --name /A
ndndpdk-godemo pingclient --name /A

You can see packets flowing through.
Run `ls -l /run/ndn` and check the uid:gid of socket files too.


>
> > Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com>
>
> <...>
>
> > @@ -1827,47 +1859,58 @@ rte_pmd_memif_probe(struct rte_vdev_device
*vdev)
> >       flags |= ETH_MEMIF_FLAG_SOCKET_ABSTRACT;
> >
> >       kvlist = rte_kvargs_parse(rte_vdev_device_args(vdev),
valid_arguments);
> > +     if (kvlist == NULL) {
> > +             MIF_LOG(ERR, "Invalid kvargs key");
> > +             ret = -EINVAL;
> > +             goto exit;
> > +     }
>
> Thanks Junxiao for updating this, but since it is not really related to
> this patch, can you please separate it to another patch?

These are reverted and will be submitted separately in the future.
  
Ferruh Yigit Dec. 8, 2022, 2:29 p.m. UTC | #4
On 12/7/2022 5:48 PM, Junxiao Shi wrote:
> Hi Ferruh
> 
>> On 12/7/2022 2:41 PM, Junxiao Shi wrote:
>> > This allows a DPDK application running with root privilege to create a
>> > memif socket listener with non-root owner uid and gid, which can be
>> > connected from client applications running without root privilege.
>> >
>>
>> Do you have an easy way to test unprivileged memif client?
> 
> This has been tested with NDN-DPDK software.
> https://github.com/usnistgov/ndn-dpdk
> <https://github.com/usnistgov/ndn-dpdk> revision
> 311de078aa4dc3ea28db5f8858e70a1bef7b9ccd
> 

Thanks for the info.

Do you want this project to be included in DPDK web page [1], if so you
can request this in web mail list (web@dpdk.org).

[1]
https://www.dpdk.org/ecosystem/#projects


> The systemd service is running as root and it uses DPDK with the
> owner-uid and owner-gid args.
> The ndndpdk-godemo command is running as unprivileged process.
> Directory /run/ndn still needs to be created by root.
> 
> These commands can perform a full test:
> 
> git clone https://github.com/usnistgov/ndn-dpdk.git
> <https://github.com/usnistgov/ndn-dpdk.git>
> cd ndn-dpdk
> ./docs/ndndpdk-depends.sh --dpdk-patch=26031
> corepack pnpm install
> make
> sudo make install
> sudo dpdk-hugepages.py --setup 8G
> sudo ndndpdk-ctrl systemd start
> jq -n {} | ndndpdk-ctrl activate-forwarder
> sudo mkdir -p /run/ndn
> ndndpdk-godemo pingserver --name /A
> ndndpdk-godemo pingclient --name /A
> 
> You can see packets flowing through.
> Run `ls -l /run/ndn` and check the uid:gid of socket files too.
> 

It is good to record these steps, but for now I will rely on your
testing :), thanks.

> 
>>
>> > Signed-off-by: Junxiao Shi <git@mail1.yoursunny.com
> <mailto:git@mail1.yoursunny.com>>
>>
>> <...>
>>
>> > @@ -1827,47 +1859,58 @@ rte_pmd_memif_probe(struct rte_vdev_device
> *vdev)
>> >       flags |= ETH_MEMIF_FLAG_SOCKET_ABSTRACT;
>> >
>> >       kvlist = rte_kvargs_parse(rte_vdev_device_args(vdev),
> valid_arguments);
>> > +     if (kvlist == NULL) {
>> > +             MIF_LOG(ERR, "Invalid kvargs key");
>> > +             ret = -EINVAL;
>> > +             goto exit;
>> > +     }
>>
>> Thanks Junxiao for updating this, but since it is not really related to
>> this patch, can you please separate it to another patch?
> 
> These are reverted and will be submitted separately in the future.

ack
  

Patch

diff --git a/doc/guides/nics/memif.rst b/doc/guides/nics/memif.rst
index aca843640b..afc574fdaa 100644
--- a/doc/guides/nics/memif.rst
+++ b/doc/guides/nics/memif.rst
@@ -44,6 +44,8 @@  client.
    "rsize=11", "Log2 of ring size. If rsize is 10, actual ring size is 1024", "10", "1-14"
    "socket=/tmp/memif.sock", "Socket filename", "/tmp/memif.sock", "string len 108"
    "socket-abstract=no", "Set usage of abstract socket address", "yes", "yes|no"
+   "owner-uid=1000", "Set socket listener owner uid. Only relevant to server with socket-abstract=no", "unchanged", "uid_t"
+   "owner-gid=1000", "Set socket listener owner gid. Only relevant to server with socket-abstract=no", "unchanged", "gid_t"
    "mac=01:23:45:ab:cd:ef", "Mac address", "01:ab:23:cd:45:ef", ""
    "secret=abc123", "Secret is an optional security option, which if specified, must be matched by peer", "", "string len 24"
    "zero-copy=yes", "Enable/disable zero-copy client mode. Only relevant to client, requires '--single-file-segments' eal argument", "no", "yes|no"
diff --git a/drivers/net/memif/memif_socket.c b/drivers/net/memif/memif_socket.c
index 4700ce2e77..649f8d0e61 100644
--- a/drivers/net/memif/memif_socket.c
+++ b/drivers/net/memif/memif_socket.c
@@ -889,7 +889,7 @@  memif_listener_handler(void *arg)
 }
 
 static struct memif_socket *
-memif_socket_create(char *key, uint8_t listener, bool is_abstract)
+memif_socket_create(char *key, uint8_t listener, bool is_abstract, uid_t owner_uid, gid_t owner_gid)
 {
 	struct memif_socket *sock;
 	struct sockaddr_un un = { 0 };
@@ -941,6 +941,14 @@  memif_socket_create(char *key, uint8_t listener, bool is_abstract)
 
 		MIF_LOG(DEBUG, "Memif listener socket %s created.", sock->filename);
 
+		if (!is_abstract && (owner_uid != (uid_t)-1 || owner_gid != (gid_t)-1)) {
+			ret = chown(sock->filename, owner_uid, owner_gid);
+			if (ret < 0) {
+				MIF_LOG(ERR, "Failed to change listener socket owner");
+				goto error;
+			}
+		}
+
 		/* Allocate interrupt instance */
 		sock->intr_handle =
 			rte_intr_instance_alloc(RTE_INTR_INSTANCE_F_SHARED);
@@ -1017,7 +1025,8 @@  memif_socket_init(struct rte_eth_dev *dev, const char *socket_filename)
 	if (ret < 0) {
 		socket = memif_socket_create(key,
 			(pmd->role == MEMIF_ROLE_CLIENT) ? 0 : 1,
-			pmd->flags & ETH_MEMIF_FLAG_SOCKET_ABSTRACT);
+			pmd->flags & ETH_MEMIF_FLAG_SOCKET_ABSTRACT,
+			pmd->owner_uid, pmd->owner_gid);
 		if (socket == NULL)
 			return -1;
 		ret = rte_hash_add_key_data(hash, key, socket);
diff --git a/drivers/net/memif/rte_eth_memif.c b/drivers/net/memif/rte_eth_memif.c
index 1b1c1a652b..871a2bd7d3 100644
--- a/drivers/net/memif/rte_eth_memif.c
+++ b/drivers/net/memif/rte_eth_memif.c
@@ -37,6 +37,8 @@ 
 #define ETH_MEMIF_RING_SIZE_ARG		"rsize"
 #define ETH_MEMIF_SOCKET_ARG		"socket"
 #define ETH_MEMIF_SOCKET_ABSTRACT_ARG	"socket-abstract"
+#define ETH_MEMIF_OWNER_UID_ARG		"owner-uid"
+#define ETH_MEMIF_OWNER_GID_ARG		"owner-gid"
 #define ETH_MEMIF_MAC_ARG		"mac"
 #define ETH_MEMIF_ZC_ARG		"zero-copy"
 #define ETH_MEMIF_SECRET_ARG		"secret"
@@ -48,6 +50,8 @@  static const char * const valid_arguments[] = {
 	ETH_MEMIF_RING_SIZE_ARG,
 	ETH_MEMIF_SOCKET_ARG,
 	ETH_MEMIF_SOCKET_ABSTRACT_ARG,
+	ETH_MEMIF_OWNER_UID_ARG,
+	ETH_MEMIF_OWNER_GID_ARG,
 	ETH_MEMIF_MAC_ARG,
 	ETH_MEMIF_ZC_ARG,
 	ETH_MEMIF_SECRET_ARG,
@@ -1515,7 +1519,7 @@  static const struct eth_dev_ops ops = {
 static int
 memif_create(struct rte_vdev_device *vdev, enum memif_role_t role,
 	     memif_interface_id_t id, uint32_t flags,
-	     const char *socket_filename,
+	     const char *socket_filename, uid_t owner_uid, gid_t owner_gid,
 	     memif_log2_ring_size_t log2_ring_size,
 	     uint16_t pkt_buffer_size, const char *secret,
 	     struct rte_ether_addr *ether_addr)
@@ -1554,6 +1558,8 @@  memif_create(struct rte_vdev_device *vdev, enum memif_role_t role,
 	/* Zero-copy flag irelevant to server. */
 	if (pmd->role == MEMIF_ROLE_SERVER)
 		pmd->flags &= ~ETH_MEMIF_FLAG_ZERO_COPY;
+	pmd->owner_uid = owner_uid;
+	pmd->owner_gid = owner_gid;
 
 	ret = memif_socket_init(eth_dev, socket_filename);
 	if (ret < 0)
@@ -1740,6 +1746,30 @@  memif_set_is_socket_abstract(const char *key __rte_unused, const char *value, vo
 	return 0;
 }
 
+static int
+memif_set_owner(const char *key, const char *value, void *extra_args)
+{
+	RTE_ASSERT(sizeof(uid_t) == sizeof(uint32_t));
+	RTE_ASSERT(sizeof(gid_t) == sizeof(uint32_t));
+
+	unsigned long val;
+	char *end = NULL;
+	uint32_t *id = (uint32_t *)extra_args;
+
+	val = strtoul(value, &end, 10);
+	if (*value == '\0' || *end != '\0') {
+		MIF_LOG(ERR, "Failed to parse %s: %s.", key, value);
+		return -EINVAL;
+	}
+	if (val >= UINT32_MAX) {
+		MIF_LOG(ERR, "Invalid %s: %s.", key, value);
+		return -ERANGE;
+	}
+
+	*id = val;
+	return 0;
+}
+
 static int
 memif_set_mac(const char *key __rte_unused, const char *value, void *extra_args)
 {
@@ -1772,6 +1802,8 @@  rte_pmd_memif_probe(struct rte_vdev_device *vdev)
 	uint16_t pkt_buffer_size = ETH_MEMIF_DEFAULT_PKT_BUFFER_SIZE;
 	memif_log2_ring_size_t log2_ring_size = ETH_MEMIF_DEFAULT_RING_SIZE;
 	const char *socket_filename = ETH_MEMIF_DEFAULT_SOCKET_FILENAME;
+	uid_t owner_uid = -1;
+	gid_t owner_gid = -1;
 	uint32_t flags = 0;
 	const char *secret = NULL;
 	struct rte_ether_addr *ether_addr = rte_zmalloc("",
@@ -1827,47 +1859,58 @@  rte_pmd_memif_probe(struct rte_vdev_device *vdev)
 	flags |= ETH_MEMIF_FLAG_SOCKET_ABSTRACT;
 
 	kvlist = rte_kvargs_parse(rte_vdev_device_args(vdev), valid_arguments);
+	if (kvlist == NULL) {
+		MIF_LOG(ERR, "Invalid kvargs key");
+		ret = -EINVAL;
+		goto exit;
+	}
 
 	/* parse parameters */
-	if (kvlist != NULL) {
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_ROLE_ARG,
-					 &memif_set_role, &role);
-		if (ret < 0)
-			goto exit;
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_ID_ARG,
-					 &memif_set_id, &id);
-		if (ret < 0)
-			goto exit;
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_PKT_BUFFER_SIZE_ARG,
-					 &memif_set_bs, &pkt_buffer_size);
-		if (ret < 0)
-			goto exit;
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_RING_SIZE_ARG,
-					 &memif_set_rs, &log2_ring_size);
-		if (ret < 0)
-			goto exit;
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_SOCKET_ARG,
-					 &memif_set_socket_filename,
-					 (void *)(&socket_filename));
-		if (ret < 0)
-			goto exit;
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_SOCKET_ABSTRACT_ARG,
-					 &memif_set_is_socket_abstract, &flags);
-		if (ret < 0)
-			goto exit;
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_MAC_ARG,
-					 &memif_set_mac, ether_addr);
-		if (ret < 0)
-			goto exit;
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_ZC_ARG,
-					 &memif_set_zc, &flags);
-		if (ret < 0)
-			goto exit;
-		ret = rte_kvargs_process(kvlist, ETH_MEMIF_SECRET_ARG,
-					 &memif_set_secret, (void *)(&secret));
-		if (ret < 0)
-			goto exit;
-	}
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_ROLE_ARG,
+					&memif_set_role, &role);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_ID_ARG,
+					&memif_set_id, &id);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_PKT_BUFFER_SIZE_ARG,
+					&memif_set_bs, &pkt_buffer_size);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_RING_SIZE_ARG,
+					&memif_set_rs, &log2_ring_size);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_SOCKET_ARG,
+					&memif_set_socket_filename,
+					(void *)(&socket_filename));
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_SOCKET_ABSTRACT_ARG,
+					&memif_set_is_socket_abstract, &flags);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_OWNER_UID_ARG,
+					&memif_set_owner, &owner_uid);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_OWNER_GID_ARG,
+					&memif_set_owner, &owner_gid);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_MAC_ARG,
+					&memif_set_mac, ether_addr);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_ZC_ARG,
+					&memif_set_zc, &flags);
+	if (ret < 0)
+		goto exit;
+	ret = rte_kvargs_process(kvlist, ETH_MEMIF_SECRET_ARG,
+					&memif_set_secret, (void *)(&secret));
+	if (ret < 0)
+		goto exit;
 
 	if (!(flags & ETH_MEMIF_FLAG_SOCKET_ABSTRACT)) {
 		ret = memif_check_socket_filename(socket_filename);
@@ -1876,7 +1919,7 @@  rte_pmd_memif_probe(struct rte_vdev_device *vdev)
 	}
 
 	/* create interface */
-	ret = memif_create(vdev, role, id, flags, socket_filename,
+	ret = memif_create(vdev, role, id, flags, socket_filename, owner_uid, owner_gid,
 			   log2_ring_size, pkt_buffer_size, secret, ether_addr);
 
 exit:
@@ -1909,7 +1952,9 @@  RTE_PMD_REGISTER_PARAM_STRING(net_memif,
 			      ETH_MEMIF_PKT_BUFFER_SIZE_ARG "=<int>"
 			      ETH_MEMIF_RING_SIZE_ARG "=<int>"
 			      ETH_MEMIF_SOCKET_ARG "=<string>"
-				  ETH_MEMIF_SOCKET_ABSTRACT_ARG "=yes|no"
+			      ETH_MEMIF_SOCKET_ABSTRACT_ARG "=yes|no"
+			      ETH_MEMIF_OWNER_UID_ARG "=<int>"
+			      ETH_MEMIF_OWNER_GID_ARG "=<int>"
 			      ETH_MEMIF_MAC_ARG "=xx:xx:xx:xx:xx:xx"
 			      ETH_MEMIF_ZC_ARG "=yes|no"
 			      ETH_MEMIF_SECRET_ARG "=<string>");
diff --git a/drivers/net/memif/rte_eth_memif.h b/drivers/net/memif/rte_eth_memif.h
index eb692aee68..6ab7b967a5 100644
--- a/drivers/net/memif/rte_eth_memif.h
+++ b/drivers/net/memif/rte_eth_memif.h
@@ -89,6 +89,8 @@  struct pmd_internals {
 /**< use abstract socket address */
 
 	char *socket_filename;			/**< pointer to socket filename */
+	uid_t owner_uid;			/**< socket owner uid */
+	gid_t owner_gid;			/**< socket owner gid */
 	char secret[ETH_MEMIF_SECRET_SIZE]; /**< secret (optional security parameter) */
 
 	struct memif_control_channel *cc;	/**< control channel */