diff mbox series

[RFC,v1,04/10] net: qrtr: Report sender endpoint in aux data

Message ID 20241018181842.1368394-5-denkenz@gmail.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series QRTR Multi-endpoint support | expand

Checks

Context Check Description
netdev/series_format warning Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 70 this patch: 70
netdev/build_tools success Errors and warnings before: 0 (+1) this patch: 0 (+1)
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1021 this patch: 1021
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 14619 this patch: 14619
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 152 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 1 this patch: 1
netdev/source_inline success Was 0 now: 0

Commit Message

Denis Kenzior Oct. 18, 2024, 6:18 p.m. UTC 
Introduce support for reporting the remote endpoint that generated a
given QRTR message to clients using AF_QIPCRTR family sockets. This is
achieved by including QRTR_ENDPOINT auxiliary data, which carries the
endpoint identifier of the message sender.  To receive this auxiliary
data, clients must explicitly opt-in by using setsockopt with the
QRTR_REPORT_ENDPOINT option enabled.

Implementation of getsockopt and setsockopt is provided.  An additional
level 'SOL_QRTR' is added to socket.h for use by AF_QIPCRTR family
sockets.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
Reviewed-by: Marcel Holtmann <marcel@holtmann.org>
Reviewed-by: Andy Gross <agross@kernel.org>
---
 include/linux/socket.h    |  1 +
 include/uapi/linux/qrtr.h |  6 +++
 net/qrtr/af_qrtr.c        | 87 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)

Comments

Kuniyuki Iwashima Oct. 19, 2024, 12:22 a.m. UTC | #1 
From: Denis Kenzior <denkenz@gmail.com>
Date: Fri, 18 Oct 2024 13:18:22 -0500
> @@ -1234,6 +1247,78 @@ static int qrtr_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>  	return rc;
>  }
>  
> +static int qrtr_setsockopt(struct socket *sock, int level, int optname,
> +			   sockptr_t optval, unsigned int optlen)
> +{
> +	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
> +	struct sock *sk = sock->sk;
> +	unsigned int val = 0;
> +	int rc = 0;
> +
> +	if (level != SOL_QRTR)
> +		return -ENOPROTOOPT;
> +
> +	if (optlen >= sizeof(val) &&
> +	    copy_from_sockptr(&val, optval, sizeof(val)))
> +		return -EFAULT;
> +
> +	lock_sock(sk);

This seems unnecessary to me.

sk_setsockopt(), do_ip_setsockopt(), and do_ipv6_setsockopt() do not
hold lock_sock() for assign_bit().

Also, QRTR_BIND_ENDPOINT in a later patch will not need lock_sock()
neither.  The value is u32, so you can use WRITE_ONCE() here and
READ_ONCE() in getsockopt().


> +
> +	switch (optname) {
> +	case QRTR_REPORT_ENDPOINT:
> +		assign_bit(QRTR_F_REPORT_ENDPOINT, &ipc->flags, val);
> +		break;
> +	default:
> +		rc = -ENOPROTOOPT;
> +	}
> +
> +	release_sock(sk);
> +
> +	return rc;
> +}
> +
> +static int qrtr_getsockopt(struct socket *sock, int level, int optname,
> +			   char __user *optval, int __user *optlen)
> +{
> +	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
> +	struct sock *sk = sock->sk;
> +	unsigned int val;
> +	int len;
> +	int rc = 0;
> +
> +	if (level != SOL_QRTR)
> +		return -ENOPROTOOPT;
> +
> +	if (get_user(len, optlen))
> +		return -EFAULT;
> +
> +	if (len < sizeof(val))
> +		return -EINVAL;
> +
> +	lock_sock(sk);

Same remark.


> +
> +	switch (optname) {
> +	case QRTR_REPORT_ENDPOINT:
> +		val = test_bit(QRTR_F_REPORT_ENDPOINT, &ipc->flags);
> +		break;
> +	default:
> +		rc = -ENOPROTOOPT;
> +	}
> +
> +	release_sock(sk);
> +
> +	if (rc)
> +		return rc;
> +
> +	len = sizeof(int);
> +
> +	if (put_user(len, optlen) ||
> +	    copy_to_user(optval, &val, len))
> +		rc = -EFAULT;
> +
> +	return rc;
> +}
> +
>  static int qrtr_release(struct socket *sock)
>  {
>  	struct sock *sk = sock->sk;
Denis Kenzior Oct. 22, 2024, 3:07 p.m. UTC | #2 
On 10/18/24 7:22 PM, Kuniyuki Iwashima wrote:
> From: Denis Kenzior <denkenz@gmail.com>
> Date: Fri, 18 Oct 2024 13:18:22 -0500
>> @@ -1234,6 +1247,78 @@ static int qrtr_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>>   	return rc;
>>   }
>>   
>> +static int qrtr_setsockopt(struct socket *sock, int level, int optname,
>> +			   sockptr_t optval, unsigned int optlen)
>> +{
>> +	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
>> +	struct sock *sk = sock->sk;
>> +	unsigned int val = 0;
>> +	int rc = 0;
>> +
>> +	if (level != SOL_QRTR)
>> +		return -ENOPROTOOPT;
>> +
>> +	if (optlen >= sizeof(val) &&
>> +	    copy_from_sockptr(&val, optval, sizeof(val)))
>> +		return -EFAULT;
>> +
>> +	lock_sock(sk);
> 
> This seems unnecessary to me.
> 
> sk_setsockopt(), do_ip_setsockopt(), and do_ipv6_setsockopt() do not
> hold lock_sock() for assign_bit().

Indeed, thanks for spotting that.  I'll fix this in the next version.  I'll also 
drop lock_sock/release_sock in qrtr_sock_set_report_endpoint (patch 9).

> 
> Also, QRTR_BIND_ENDPOINT in a later patch will not need lock_sock()
> neither.  The value is u32, so you can use WRITE_ONCE() here and
> READ_ONCE() in getsockopt().
> 

Makes sense, I'll fix this as well.

> 
>> +
>> +	switch (optname) {
>> +	case QRTR_REPORT_ENDPOINT:
>> +		assign_bit(QRTR_F_REPORT_ENDPOINT, &ipc->flags, val);
>> +		break;
>> +	default:
>> +		rc = -ENOPROTOOPT;
>> +	}
>> +
>> +	release_sock(sk);
>> +
>> +	return rc;
>> +}
>> +
>> +static int qrtr_getsockopt(struct socket *sock, int level, int optname,
>> +			   char __user *optval, int __user *optlen)
>> +{
>> +	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
>> +	struct sock *sk = sock->sk;
>> +	unsigned int val;
>> +	int len;
>> +	int rc = 0;
>> +
>> +	if (level != SOL_QRTR)
>> +		return -ENOPROTOOPT;
>> +
>> +	if (get_user(len, optlen))
>> +		return -EFAULT;
>> +
>> +	if (len < sizeof(val))
>> +		return -EINVAL;
>> +
>> +	lock_sock(sk);
> 
> Same remark.
> 
> 
>> +
>> +	switch (optname) {
>> +	case QRTR_REPORT_ENDPOINT:
>> +		val = test_bit(QRTR_F_REPORT_ENDPOINT, &ipc->flags);
>> +		break;
>> +	default:
>> +		rc = -ENOPROTOOPT;
>> +	}
>> +
>> +	release_sock(sk);
>> +
>> +	if (rc)
>> +		return rc;
>> +
>> +	len = sizeof(int);
>> +
>> +	if (put_user(len, optlen) ||
>> +	    copy_to_user(optval, &val, len))
>> +		rc = -EFAULT;
>> +
>> +	return rc;
>> +}
>> +
>>   static int qrtr_release(struct socket *sock)
>>   {
>>   	struct sock *sk = sock->sk;
diff mbox series

Patch

diff --git a/include/linux/socket.h b/include/linux/socket.h
index d18cc47e89bd..7491884340cf 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -386,6 +386,7 @@  struct ucred {
 #define SOL_MCTP	285
 #define SOL_SMC		286
 #define SOL_VSOCK	287
+#define SOL_QRTR	288
 
 /* IPX options */
 #define IPX_TYPE	1
diff --git a/include/uapi/linux/qrtr.h b/include/uapi/linux/qrtr.h
index f7e2fb3d752b..6d0911984a05 100644
--- a/include/uapi/linux/qrtr.h
+++ b/include/uapi/linux/qrtr.h
@@ -46,4 +46,10 @@  struct qrtr_ctrl_pkt {
 	};
 } __packed;
 
+/* setsockopt / getsockopt */
+#define QRTR_REPORT_ENDPOINT 1
+
+/* CMSG */
+#define QRTR_ENDPOINT 1
+
 #endif /* _LINUX_QRTR_H */
diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index e83d491a8da9..cb7bd1c71e6d 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -26,6 +26,10 @@ 
 
 #define QRTR_PORT_CTRL_LEGACY 0xffff
 
+enum {
+	QRTR_F_REPORT_ENDPOINT,
+};
+
 /**
  * struct qrtr_hdr_v1 - (I|R)PCrouter packet header version 1
  * @version: protocol version
@@ -79,6 +83,7 @@  struct qrtr_cb {
 	u32 src_port;
 	u32 dst_node;
 	u32 dst_port;
+	u32 endpoint_id;
 
 	u8 type;
 	u8 confirm_rx;
@@ -92,6 +97,7 @@  struct qrtr_sock {
 	struct sock sk;
 	struct sockaddr_qrtr us;
 	struct sockaddr_qrtr peer;
+	unsigned long flags;
 };
 
 static inline struct qrtr_sock *qrtr_sk(struct sock *sk)
@@ -513,6 +519,8 @@  int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
 	if (cb->dst_port == QRTR_PORT_CTRL_LEGACY)
 		cb->dst_port = QRTR_PORT_CTRL;
 
+	cb->endpoint_id = ep->id;
+
 	if (!size || len != ALIGN(size, 4) + hdrlen)
 		goto err;
 
@@ -1064,6 +1072,7 @@  static int qrtr_recvmsg(struct socket *sock, struct msghdr *msg,
 			size_t size, int flags)
 {
 	DECLARE_SOCKADDR(struct sockaddr_qrtr *, addr, msg->msg_name);
+	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
 	struct sock *sk = sock->sk;
 	struct sk_buff *skb;
 	struct qrtr_cb *cb;
@@ -1089,6 +1098,10 @@  static int qrtr_recvmsg(struct socket *sock, struct msghdr *msg,
 		msg->msg_flags |= MSG_TRUNC;
 	}
 
+	if (cb->endpoint_id && test_bit(QRTR_F_REPORT_ENDPOINT, &ipc->flags))
+		put_cmsg(msg, SOL_QRTR, QRTR_ENDPOINT,
+			 sizeof(cb->endpoint_id), &cb->endpoint_id);
+
 	rc = skb_copy_datagram_msg(skb, 0, msg, copied);
 	if (rc < 0)
 		goto out;
@@ -1234,6 +1247,78 @@  static int qrtr_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 	return rc;
 }
 
+static int qrtr_setsockopt(struct socket *sock, int level, int optname,
+			   sockptr_t optval, unsigned int optlen)
+{
+	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
+	struct sock *sk = sock->sk;
+	unsigned int val = 0;
+	int rc = 0;
+
+	if (level != SOL_QRTR)
+		return -ENOPROTOOPT;
+
+	if (optlen >= sizeof(val) &&
+	    copy_from_sockptr(&val, optval, sizeof(val)))
+		return -EFAULT;
+
+	lock_sock(sk);
+
+	switch (optname) {
+	case QRTR_REPORT_ENDPOINT:
+		assign_bit(QRTR_F_REPORT_ENDPOINT, &ipc->flags, val);
+		break;
+	default:
+		rc = -ENOPROTOOPT;
+	}
+
+	release_sock(sk);
+
+	return rc;
+}
+
+static int qrtr_getsockopt(struct socket *sock, int level, int optname,
+			   char __user *optval, int __user *optlen)
+{
+	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
+	struct sock *sk = sock->sk;
+	unsigned int val;
+	int len;
+	int rc = 0;
+
+	if (level != SOL_QRTR)
+		return -ENOPROTOOPT;
+
+	if (get_user(len, optlen))
+		return -EFAULT;
+
+	if (len < sizeof(val))
+		return -EINVAL;
+
+	lock_sock(sk);
+
+	switch (optname) {
+	case QRTR_REPORT_ENDPOINT:
+		val = test_bit(QRTR_F_REPORT_ENDPOINT, &ipc->flags);
+		break;
+	default:
+		rc = -ENOPROTOOPT;
+	}
+
+	release_sock(sk);
+
+	if (rc)
+		return rc;
+
+	len = sizeof(int);
+
+	if (put_user(len, optlen) ||
+	    copy_to_user(optval, &val, len))
+		rc = -EFAULT;
+
+	return rc;
+}
+
 static int qrtr_release(struct socket *sock)
 {
 	struct sock *sk = sock->sk;
@@ -1281,6 +1366,8 @@  static const struct proto_ops qrtr_proto_ops = {
 	.shutdown	= sock_no_shutdown,
 	.release	= qrtr_release,
 	.mmap		= sock_no_mmap,
+	.setsockopt	= qrtr_setsockopt,
+	.getsockopt	= qrtr_getsockopt,
 };
 
 static struct proto qrtr_proto = {