All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] Bug fixes for net/handshake
@ 2023-05-04 15:24 Chuck Lever
  2023-05-04 15:24 ` [PATCH 1/5] net/handshake: Remove unneeded check from handshake_dup() Chuck Lever
                   ` (5 more replies)
  0 siblings, 6 replies; 18+ messages in thread
From: Chuck Lever @ 2023-05-04 15:24 UTC (permalink / raw)
  To: kernel-tls-handshake; +Cc: netdev, dan.carpenter

I plan to send these as part of a 6.4-rc PR.

---

Chuck Lever (5):
      net/handshake: Remove unneeded check from handshake_dup()
      net/handshake: Fix handshake_dup() ref counting
      net/handshake: Fix uninitialized local variable
      net/handshake: handshake_genl_notify() shouldn't ignore @flags
      net/handshake: Enable the SNI extension to work properly


 Documentation/netlink/specs/handshake.yaml |  4 ++++
 Documentation/networking/tls-handshake.rst |  5 +++++
 include/net/handshake.h                    |  1 +
 include/uapi/linux/handshake.h             |  1 +
 net/handshake/netlink.c                    | 17 +++++------------
 net/handshake/tlshd.c                      |  8 ++++++++
 6 files changed, 24 insertions(+), 12 deletions(-)

--
Chuck Lever


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/5] net/handshake: Remove unneeded check from handshake_dup()
  2023-05-04 15:24 [PATCH 0/5] Bug fixes for net/handshake Chuck Lever
@ 2023-05-04 15:24 ` Chuck Lever
  2023-05-05 13:15   ` Simon Horman
  2023-05-04 15:25 ` [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting Chuck Lever
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Chuck Lever @ 2023-05-04 15:24 UTC (permalink / raw)
  To: kernel-tls-handshake; +Cc: netdev, dan.carpenter

From: Chuck Lever <chuck.lever@oracle.com>

handshake_req_submit() now verifies that the socket has a file.

Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/netlink.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 35c9c445e0b8..7ec8a76c3c8a 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -99,9 +99,6 @@ static int handshake_dup(struct socket *sock)
 	struct file *file;
 	int newfd;
 
-	if (!sock->file)
-		return -EBADF;
-
 	file = get_file(sock->file);
 	newfd = get_unused_fd_flags(O_CLOEXEC);
 	if (newfd < 0) {



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting
  2023-05-04 15:24 [PATCH 0/5] Bug fixes for net/handshake Chuck Lever
  2023-05-04 15:24 ` [PATCH 1/5] net/handshake: Remove unneeded check from handshake_dup() Chuck Lever
@ 2023-05-04 15:25 ` Chuck Lever
  2023-05-05 13:15   ` Simon Horman
  2023-05-05 20:58   ` Jakub Kicinski
  2023-05-04 15:25 ` [PATCH 3/5] net/handshake: Fix uninitialized local variable Chuck Lever
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 18+ messages in thread
From: Chuck Lever @ 2023-05-04 15:25 UTC (permalink / raw)
  To: kernel-tls-handshake; +Cc: netdev, dan.carpenter

From: Chuck Lever <chuck.lever@oracle.com>

If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
twice.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/netlink.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 7ec8a76c3c8a..3508bc3e661d 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -96,17 +96,13 @@ EXPORT_SYMBOL(handshake_genl_put);
  */
 static int handshake_dup(struct socket *sock)
 {
-	struct file *file;
 	int newfd;
 
-	file = get_file(sock->file);
 	newfd = get_unused_fd_flags(O_CLOEXEC);
-	if (newfd < 0) {
-		fput(file);
+	if (newfd < 0)
 		return newfd;
-	}
 
-	fd_install(newfd, file);
+	fd_install(newfd, sock->file);
 	return newfd;
 }
 
@@ -143,11 +139,11 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
 		goto out_complete;
 
 	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
+	get_file(sock->file);	/* released by DONE */
 	return 0;
 
 out_complete:
 	handshake_complete(req, -EIO, NULL);
-	fput(sock->file);
 out_status:
 	trace_handshake_cmd_accept_err(net, req, NULL, err);
 	return err;



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 3/5] net/handshake: Fix uninitialized local variable
  2023-05-04 15:24 [PATCH 0/5] Bug fixes for net/handshake Chuck Lever
  2023-05-04 15:24 ` [PATCH 1/5] net/handshake: Remove unneeded check from handshake_dup() Chuck Lever
  2023-05-04 15:25 ` [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting Chuck Lever
@ 2023-05-04 15:25 ` Chuck Lever
  2023-05-05 13:15   ` Simon Horman
  2023-05-04 15:25 ` [PATCH 4/5] net/handshake: handshake_genl_notify() shouldn't ignore @flags Chuck Lever
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 18+ messages in thread
From: Chuck Lever @ 2023-05-04 15:25 UTC (permalink / raw)
  To: kernel-tls-handshake; +Cc: netdev, dan.carpenter

From: Chuck Lever <chuck.lever@oracle.com>

trace_handshake_cmd_done_err() simply records the pointer in @req,
so initializing it to NULL is sufficient and safe.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 3508bc3e661d..8c2d13190314 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -152,8 +152,8 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
 int handshake_nl_done_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net *net = sock_net(skb->sk);
+	struct handshake_req *req = NULL;
 	struct socket *sock = NULL;
-	struct handshake_req *req;
 	int fd, status, err;
 
 	if (GENL_REQ_ATTR_CHECK(info, HANDSHAKE_A_DONE_SOCKFD))



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 4/5] net/handshake: handshake_genl_notify() shouldn't ignore @flags
  2023-05-04 15:24 [PATCH 0/5] Bug fixes for net/handshake Chuck Lever
                   ` (2 preceding siblings ...)
  2023-05-04 15:25 ` [PATCH 3/5] net/handshake: Fix uninitialized local variable Chuck Lever
@ 2023-05-04 15:25 ` Chuck Lever
  2023-05-05 13:15   ` Simon Horman
  2023-05-04 15:26 ` [PATCH 5/5] net/handshake: Enable the SNI extension to work properly Chuck Lever
  2023-05-05 20:39 ` [PATCH 0/5] Bug fixes for net/handshake Jakub Kicinski
  5 siblings, 1 reply; 18+ messages in thread
From: Chuck Lever @ 2023-05-04 15:25 UTC (permalink / raw)
  To: kernel-tls-handshake; +Cc: netdev, dan.carpenter

From: Chuck Lever <chuck.lever@oracle.com>

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/handshake/netlink.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
index 8c2d13190314..ab1ba5175c03 100644
--- a/net/handshake/netlink.c
+++ b/net/handshake/netlink.c
@@ -48,7 +48,7 @@ int handshake_genl_notify(struct net *net, const struct handshake_proto *proto,
 				proto->hp_handler_class))
 		return -ESRCH;
 
-	msg = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	msg = genlmsg_new(GENLMSG_DEFAULT_SIZE, flags);
 	if (!msg)
 		return -ENOMEM;
 



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH 5/5] net/handshake: Enable the SNI extension to work properly
  2023-05-04 15:24 [PATCH 0/5] Bug fixes for net/handshake Chuck Lever
                   ` (3 preceding siblings ...)
  2023-05-04 15:25 ` [PATCH 4/5] net/handshake: handshake_genl_notify() shouldn't ignore @flags Chuck Lever
@ 2023-05-04 15:26 ` Chuck Lever
  2023-05-05 13:16   ` Simon Horman
  2023-05-05 20:39 ` [PATCH 0/5] Bug fixes for net/handshake Jakub Kicinski
  5 siblings, 1 reply; 18+ messages in thread
From: Chuck Lever @ 2023-05-04 15:26 UTC (permalink / raw)
  To: kernel-tls-handshake; +Cc: netdev, dan.carpenter

From: Chuck Lever <chuck.lever@oracle.com>

Enable the upper layer protocol to specify the SNI peername. This
avoids the need for tlshd to use a DNS lookup, which can return a
hostname that doesn't match the incoming certificate's SubjectName.

Fixes: 2fd5532044a8 ("net/handshake: Add a kernel API for requesting a TLSv1.3 handshake")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 Documentation/netlink/specs/handshake.yaml |    4 ++++
 Documentation/networking/tls-handshake.rst |    5 +++++
 include/net/handshake.h                    |    1 +
 include/uapi/linux/handshake.h             |    1 +
 net/handshake/tlshd.c                      |    8 ++++++++
 5 files changed, 19 insertions(+)

diff --git a/Documentation/netlink/specs/handshake.yaml b/Documentation/netlink/specs/handshake.yaml
index 614f1a585511..6d89e30f5fd5 100644
--- a/Documentation/netlink/specs/handshake.yaml
+++ b/Documentation/netlink/specs/handshake.yaml
@@ -68,6 +68,9 @@ attribute-sets:
         type: nest
         nested-attributes: x509
         multi-attr: true
+      -
+        name: peername
+        type: string
   -
     name: done
     attributes:
@@ -105,6 +108,7 @@ operations:
             - auth-mode
             - peer-identity
             - certificate
+            - peername
     -
       name: done
       doc: Handler reports handshake completion
diff --git a/Documentation/networking/tls-handshake.rst b/Documentation/networking/tls-handshake.rst
index a2817a88e905..6f5ea1646a47 100644
--- a/Documentation/networking/tls-handshake.rst
+++ b/Documentation/networking/tls-handshake.rst
@@ -53,6 +53,7 @@ fills in a structure that contains the parameters of the request:
         struct socket   *ta_sock;
         tls_done_func_t ta_done;
         void            *ta_data;
+        const char      *ta_peername;
         unsigned int    ta_timeout_ms;
         key_serial_t    ta_keyring;
         key_serial_t    ta_my_cert;
@@ -71,6 +72,10 @@ instantiated a struct file in sock->file.
 has completed. Further explanation of this function is in the "Handshake
 Completion" sesction below.
 
+The consumer can provide a NUL-terminated hostname in the @ta_peername
+field that is sent as part of ClientHello. If no peername is provided,
+the DNS hostname associated with the server's IP address is used instead.
+
 The consumer can fill in the @ta_timeout_ms field to force the servicing
 handshake agent to exit after a number of milliseconds. This enables the
 socket to be fully closed once both the kernel and the handshake agent
diff --git a/include/net/handshake.h b/include/net/handshake.h
index 3352b1ab43b3..2e26e436e85f 100644
--- a/include/net/handshake.h
+++ b/include/net/handshake.h
@@ -24,6 +24,7 @@ struct tls_handshake_args {
 	struct socket		*ta_sock;
 	tls_done_func_t		ta_done;
 	void			*ta_data;
+	const char		*ta_peername;
 	unsigned int		ta_timeout_ms;
 	key_serial_t		ta_keyring;
 	key_serial_t		ta_my_cert;
diff --git a/include/uapi/linux/handshake.h b/include/uapi/linux/handshake.h
index 1de4d0b95325..3d7ea58778c9 100644
--- a/include/uapi/linux/handshake.h
+++ b/include/uapi/linux/handshake.h
@@ -44,6 +44,7 @@ enum {
 	HANDSHAKE_A_ACCEPT_AUTH_MODE,
 	HANDSHAKE_A_ACCEPT_PEER_IDENTITY,
 	HANDSHAKE_A_ACCEPT_CERTIFICATE,
+	HANDSHAKE_A_ACCEPT_PEERNAME,
 
 	__HANDSHAKE_A_ACCEPT_MAX,
 	HANDSHAKE_A_ACCEPT_MAX = (__HANDSHAKE_A_ACCEPT_MAX - 1)
diff --git a/net/handshake/tlshd.c b/net/handshake/tlshd.c
index fcbeb63b4eb1..b735f5cced2f 100644
--- a/net/handshake/tlshd.c
+++ b/net/handshake/tlshd.c
@@ -31,6 +31,7 @@ struct tls_handshake_req {
 	int			th_type;
 	unsigned int		th_timeout_ms;
 	int			th_auth_mode;
+	const char		*th_peername;
 	key_serial_t		th_keyring;
 	key_serial_t		th_certificate;
 	key_serial_t		th_privkey;
@@ -48,6 +49,7 @@ tls_handshake_req_init(struct handshake_req *req,
 	treq->th_timeout_ms = args->ta_timeout_ms;
 	treq->th_consumer_done = args->ta_done;
 	treq->th_consumer_data = args->ta_data;
+	treq->th_peername = args->ta_peername;
 	treq->th_keyring = args->ta_keyring;
 	treq->th_num_peerids = 0;
 	treq->th_certificate = TLS_NO_CERT;
@@ -214,6 +216,12 @@ static int tls_handshake_accept(struct handshake_req *req,
 	ret = nla_put_u32(msg, HANDSHAKE_A_ACCEPT_MESSAGE_TYPE, treq->th_type);
 	if (ret < 0)
 		goto out_cancel;
+	if (treq->th_peername) {
+		ret = nla_put_string(msg, HANDSHAKE_A_ACCEPT_PEERNAME,
+				     treq->th_peername);
+		if (ret < 0)
+			goto out_cancel;
+	}
 	if (treq->th_timeout_ms) {
 		ret = nla_put_u32(msg, HANDSHAKE_A_ACCEPT_TIMEOUT, treq->th_timeout_ms);
 		if (ret < 0)



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/5] net/handshake: Fix uninitialized local variable
  2023-05-04 15:25 ` [PATCH 3/5] net/handshake: Fix uninitialized local variable Chuck Lever
@ 2023-05-05 13:15   ` Simon Horman
  0 siblings, 0 replies; 18+ messages in thread
From: Simon Horman @ 2023-05-05 13:15 UTC (permalink / raw)
  To: Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Thu, May 04, 2023 at 11:25:32AM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> trace_handshake_cmd_done_err() simply records the pointer in @req,
> so initializing it to NULL is sufficient and safe.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/5] net/handshake: Remove unneeded check from handshake_dup()
  2023-05-04 15:24 ` [PATCH 1/5] net/handshake: Remove unneeded check from handshake_dup() Chuck Lever
@ 2023-05-05 13:15   ` Simon Horman
  0 siblings, 0 replies; 18+ messages in thread
From: Simon Horman @ 2023-05-05 13:15 UTC (permalink / raw)
  To: Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Thu, May 04, 2023 at 11:24:38AM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> handshake_req_submit() now verifies that the socket has a file.
> 
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting
  2023-05-04 15:25 ` [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting Chuck Lever
@ 2023-05-05 13:15   ` Simon Horman
  2023-05-05 20:58   ` Jakub Kicinski
  1 sibling, 0 replies; 18+ messages in thread
From: Simon Horman @ 2023-05-05 13:15 UTC (permalink / raw)
  To: Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Thu, May 04, 2023 at 11:25:05AM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> twice.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 4/5] net/handshake: handshake_genl_notify() shouldn't ignore @flags
  2023-05-04 15:25 ` [PATCH 4/5] net/handshake: handshake_genl_notify() shouldn't ignore @flags Chuck Lever
@ 2023-05-05 13:15   ` Simon Horman
  0 siblings, 0 replies; 18+ messages in thread
From: Simon Horman @ 2023-05-05 13:15 UTC (permalink / raw)
  To: Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Thu, May 04, 2023 at 11:25:58AM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 5/5] net/handshake: Enable the SNI extension to work properly
  2023-05-04 15:26 ` [PATCH 5/5] net/handshake: Enable the SNI extension to work properly Chuck Lever
@ 2023-05-05 13:16   ` Simon Horman
  0 siblings, 0 replies; 18+ messages in thread
From: Simon Horman @ 2023-05-05 13:16 UTC (permalink / raw)
  To: Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Thu, May 04, 2023 at 11:26:17AM -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Enable the upper layer protocol to specify the SNI peername. This
> avoids the need for tlshd to use a DNS lookup, which can return a
> hostname that doesn't match the incoming certificate's SubjectName.
> 
> Fixes: 2fd5532044a8 ("net/handshake: Add a kernel API for requesting a TLSv1.3 handshake")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5] Bug fixes for net/handshake
  2023-05-04 15:24 [PATCH 0/5] Bug fixes for net/handshake Chuck Lever
                   ` (4 preceding siblings ...)
  2023-05-04 15:26 ` [PATCH 5/5] net/handshake: Enable the SNI extension to work properly Chuck Lever
@ 2023-05-05 20:39 ` Jakub Kicinski
  2023-05-05 23:16   ` Chuck Lever
  5 siblings, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2023-05-05 20:39 UTC (permalink / raw)
  To: Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Thu, 04 May 2023 11:24:12 -0400 Chuck Lever wrote:
> I plan to send these as part of a 6.4-rc PR.

Can you elaborate?  You'll send us the same code as PR?
I'm about to send the first batch of fixes to Linus,
I was going to apply this series.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting
  2023-05-04 15:25 ` [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting Chuck Lever
  2023-05-05 13:15   ` Simon Horman
@ 2023-05-05 20:58   ` Jakub Kicinski
  2023-05-05 23:28     ` Chuck Lever
  1 sibling, 1 reply; 18+ messages in thread
From: Jakub Kicinski @ 2023-05-05 20:58 UTC (permalink / raw)
  To: Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Thu, 04 May 2023 11:25:05 -0400 Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> twice.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

> diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> index 7ec8a76c3c8a..3508bc3e661d 100644
> --- a/net/handshake/netlink.c
> +++ b/net/handshake/netlink.c
> @@ -96,17 +96,13 @@ EXPORT_SYMBOL(handshake_genl_put);
>   */
>  static int handshake_dup(struct socket *sock)
>  {
> -	struct file *file;
>  	int newfd;
>  
> -	file = get_file(sock->file);
>  	newfd = get_unused_fd_flags(O_CLOEXEC);
> -	if (newfd < 0) {
> -		fput(file);
> +	if (newfd < 0)
>  		return newfd;
> -	}
>  
> -	fd_install(newfd, file);
> +	fd_install(newfd, sock->file);

I'm not vfs expert but doesn't this mean that we will now have the file
installed in the fd table, under newfd, before we incremented the
refcount?  Can't another thread close(newfd) and make sock->file
get freed?

>  	return newfd;
>  }
>  
> @@ -143,11 +139,11 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
>  		goto out_complete;
>  
>  	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
> +	get_file(sock->file);	/* released by DONE */

What if DONE does not get called?

>  	return 0;
>  
>  out_complete:
>  	handshake_complete(req, -EIO, NULL);

We don't want to release the fd here?

> -	fput(sock->file);
>  out_status:
>  	trace_handshake_cmd_accept_err(net, req, NULL, err);
>  	return err;
-- 
pw-bot: cr

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5] Bug fixes for net/handshake
  2023-05-05 20:39 ` [PATCH 0/5] Bug fixes for net/handshake Jakub Kicinski
@ 2023-05-05 23:16   ` Chuck Lever
  2023-05-05 23:47     ` Jakub Kicinski
  0 siblings, 1 reply; 18+ messages in thread
From: Chuck Lever @ 2023-05-05 23:16 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Fri, May 05, 2023 at 01:39:18PM -0700, Jakub Kicinski wrote:
> On Thu, 04 May 2023 11:24:12 -0400 Chuck Lever wrote:
> > I plan to send these as part of a 6.4-rc PR.
> 
> Can you elaborate?  You'll send us the same code as PR?
> I'm about to send the first batch of fixes to Linus,
> I was going to apply this series.

Since I am listed as a maintainer/supporter of net/handshake, I
assumed I can and should be sending changes through nfsd or some
other repo I can commit to.

netdev@ is also listed in MAINTAINERS, so I Cc'd you all on this
series. I did not intend for you to be responsible for merging the
series. We'll need to agree on a workflow going forward.

Since you had some review questions about one of the patches,
maybe that patch should not be merged at this time by either of us.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting
  2023-05-05 20:58   ` Jakub Kicinski
@ 2023-05-05 23:28     ` Chuck Lever
  0 siblings, 0 replies; 18+ messages in thread
From: Chuck Lever @ 2023-05-05 23:28 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Fri, May 05, 2023 at 01:58:08PM -0700, Jakub Kicinski wrote:
> On Thu, 04 May 2023 11:25:05 -0400 Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > If get_unused_fd_flags() fails, we ended up calling fput(sock->file)
> > twice.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Fixes: 3b3009ea8abb ("net/handshake: Create a NETLINK service for handling handshake requests")
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> 
> > diff --git a/net/handshake/netlink.c b/net/handshake/netlink.c
> > index 7ec8a76c3c8a..3508bc3e661d 100644
> > --- a/net/handshake/netlink.c
> > +++ b/net/handshake/netlink.c
> > @@ -96,17 +96,13 @@ EXPORT_SYMBOL(handshake_genl_put);
> >   */
> >  static int handshake_dup(struct socket *sock)
> >  {
> > -	struct file *file;
> >  	int newfd;
> >  
> > -	file = get_file(sock->file);
> >  	newfd = get_unused_fd_flags(O_CLOEXEC);
> > -	if (newfd < 0) {
> > -		fput(file);
> > +	if (newfd < 0)
> >  		return newfd;
> > -	}
> >  
> > -	fd_install(newfd, file);
> > +	fd_install(newfd, sock->file);
> 
> I'm not vfs expert but doesn't this mean that we will now have the file
> installed in the fd table, under newfd, before we incremented the
> refcount?  Can't another thread close(newfd) and make sock->file
> get freed?

I suppose. I can rework it and send a refresh.


> >  	return newfd;
> >  }
> >  
> > @@ -143,11 +139,11 @@ int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
> >  		goto out_complete;
> >  
> >  	trace_handshake_cmd_accept(net, req, req->hr_sk, fd);
> > +	get_file(sock->file);	/* released by DONE */
> 
> What if DONE does not get called?

A correctly-coded kernel caller has a timeout that is supposed to
release the socket via handshake_req_cancel(). I see that it doesn't
put sock->file, though... perhaps it should use sockfd_put() instead
of sock_release().


> >  	return 0;
> >  
> >  out_complete:
> >  	handshake_complete(req, -EIO, NULL);
> 
> We don't want to release the fd here?

Not any more, since we don't do the get_file() until everything
else has succeeded. But the rework will likely restore the fput
here.


> > -	fput(sock->file);
> >  out_status:
> >  	trace_handshake_cmd_accept_err(net, req, NULL, err);
> >  	return err;
> -- 
> pw-bot: cr

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5] Bug fixes for net/handshake
  2023-05-05 23:16   ` Chuck Lever
@ 2023-05-05 23:47     ` Jakub Kicinski
  2023-05-05 23:58       ` Chuck Lever
  2023-05-08  5:51       ` Paolo Abeni
  0 siblings, 2 replies; 18+ messages in thread
From: Jakub Kicinski @ 2023-05-05 23:47 UTC (permalink / raw)
  To: Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Fri, 5 May 2023 19:16:40 -0400 Chuck Lever wrote:
> On Fri, May 05, 2023 at 01:39:18PM -0700, Jakub Kicinski wrote:
> > On Thu, 04 May 2023 11:24:12 -0400 Chuck Lever wrote:  
> > > I plan to send these as part of a 6.4-rc PR.  
> > 
> > Can you elaborate?  You'll send us the same code as PR?
> > I'm about to send the first batch of fixes to Linus,
> > I was going to apply this series.  
> 
> Since I am listed as a maintainer/supporter of net/handshake, I
> assumed I can and should be sending changes through nfsd or some
> other repo I can commit to.
> 
> netdev@ is also listed in MAINTAINERS, so I Cc'd you all on this
> series. I did not intend for you to be responsible for merging the
> series. We'll need to agree on a workflow going forward.

Let me talk to DaveM and Paolo -- with NFS being the main user
taking it via your trees is likely fine. But if it's a generic TLS
handshake and other users will appear - netdev trees may be a more
natural central point :S DaveM and Paolo are more familiar with
existing cases of similar nature (rxrpc?)..

> Since you had some review questions about one of the patches,
> maybe that patch should not be merged at this time by either of us.

Right, I noticed the cover message first then started looking more 
in depth at the code :)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5] Bug fixes for net/handshake
  2023-05-05 23:47     ` Jakub Kicinski
@ 2023-05-05 23:58       ` Chuck Lever
  2023-05-08  5:51       ` Paolo Abeni
  1 sibling, 0 replies; 18+ messages in thread
From: Chuck Lever @ 2023-05-05 23:58 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Fri, May 05, 2023 at 04:47:15PM -0700, Jakub Kicinski wrote:
> On Fri, 5 May 2023 19:16:40 -0400 Chuck Lever wrote:
> > On Fri, May 05, 2023 at 01:39:18PM -0700, Jakub Kicinski wrote:
> > > On Thu, 04 May 2023 11:24:12 -0400 Chuck Lever wrote:  
> > > > I plan to send these as part of a 6.4-rc PR.  
> > > 
> > > Can you elaborate?  You'll send us the same code as PR?
> > > I'm about to send the first batch of fixes to Linus,
> > > I was going to apply this series.  
> > 
> > Since I am listed as a maintainer/supporter of net/handshake, I
> > assumed I can and should be sending changes through nfsd or some
> > other repo I can commit to.
> > 
> > netdev@ is also listed in MAINTAINERS, so I Cc'd you all on this
> > series. I did not intend for you to be responsible for merging the
> > series. We'll need to agree on a workflow going forward.
> 
> Let me talk to DaveM and Paolo -- with NFS being the main user
> taking it via your trees is likely fine. But if it's a generic TLS
> handshake and other users will appear - netdev trees may be a more
> natural central point :S DaveM and Paolo are more familiar with
> existing cases of similar nature (rxrpc?)..

Makes sense. We expect NVMe to become a consumer in the near future,
and have considered a putative in-kernel QUIC implementation to be
another potential consumer down the road.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/5] Bug fixes for net/handshake
  2023-05-05 23:47     ` Jakub Kicinski
  2023-05-05 23:58       ` Chuck Lever
@ 2023-05-08  5:51       ` Paolo Abeni
  1 sibling, 0 replies; 18+ messages in thread
From: Paolo Abeni @ 2023-05-08  5:51 UTC (permalink / raw)
  To: Jakub Kicinski, Chuck Lever; +Cc: kernel-tls-handshake, netdev, dan.carpenter

On Fri, 2023-05-05 at 16:47 -0700, Jakub Kicinski wrote:
> On Fri, 5 May 2023 19:16:40 -0400 Chuck Lever wrote:
> > On Fri, May 05, 2023 at 01:39:18PM -0700, Jakub Kicinski wrote:
> > > On Thu, 04 May 2023 11:24:12 -0400 Chuck Lever wrote:  
> > > > I plan to send these as part of a 6.4-rc PR.  
> > > 
> > > Can you elaborate?  You'll send us the same code as PR?
> > > I'm about to send the first batch of fixes to Linus,
> > > I was going to apply this series.  
> > 
> > Since I am listed as a maintainer/supporter of net/handshake, I
> > assumed I can and should be sending changes through nfsd or some
> > other repo I can commit to.
> > 
> > netdev@ is also listed in MAINTAINERS, so I Cc'd you all on this
> > series. I did not intend for you to be responsible for merging the
> > series. We'll need to agree on a workflow going forward.
> 
> Let me talk to DaveM and Paolo -- with NFS being the main user
> taking it via your trees is likely fine. But if it's a generic TLS
> handshake and other users will appear - netdev trees may be a more
> natural central point :S DaveM and Paolo are more familiar with
> existing cases of similar nature (rxrpc?)..

Really, I' not ;)

My guess is that net/handshake is going to be dependent more on core
networking changes than anything else. If later developments will
require/use/leverage a new core net helper, it would be quite straight-
forward going trough the netdev trees. Otherwise such changes will
require extra coordination and/or an additional RTT WRT kernel
releases.

All the above very much IMHO ;)

/P


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-05-08  5:51 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-04 15:24 [PATCH 0/5] Bug fixes for net/handshake Chuck Lever
2023-05-04 15:24 ` [PATCH 1/5] net/handshake: Remove unneeded check from handshake_dup() Chuck Lever
2023-05-05 13:15   ` Simon Horman
2023-05-04 15:25 ` [PATCH 2/5] net/handshake: Fix handshake_dup() ref counting Chuck Lever
2023-05-05 13:15   ` Simon Horman
2023-05-05 20:58   ` Jakub Kicinski
2023-05-05 23:28     ` Chuck Lever
2023-05-04 15:25 ` [PATCH 3/5] net/handshake: Fix uninitialized local variable Chuck Lever
2023-05-05 13:15   ` Simon Horman
2023-05-04 15:25 ` [PATCH 4/5] net/handshake: handshake_genl_notify() shouldn't ignore @flags Chuck Lever
2023-05-05 13:15   ` Simon Horman
2023-05-04 15:26 ` [PATCH 5/5] net/handshake: Enable the SNI extension to work properly Chuck Lever
2023-05-05 13:16   ` Simon Horman
2023-05-05 20:39 ` [PATCH 0/5] Bug fixes for net/handshake Jakub Kicinski
2023-05-05 23:16   ` Chuck Lever
2023-05-05 23:47     ` Jakub Kicinski
2023-05-05 23:58       ` Chuck Lever
2023-05-08  5:51       ` Paolo Abeni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.