Commit 1af2af70 authored by Paolo Abeni's avatar Paolo Abeni
Browse files

Merge branch 'net-handshake-anchor-request-lifetime-to-a-pinned-file-reference' 

Chuck Lever says:

====================
net/handshake: anchor request lifetime to a pinned file reference

handshake_nl_accept_doit() has accumulated four follow-on fixes
since 3b3009ea ("net/handshake: Create a NETLINK service for
handling handshake requests"): 7ea9c1ec, 7798b594,
fe67b063, and dabac51b.  Each was a local refcount or
NULL-check correction; none moved where the file reference is
owned, and the same code keeps producing the same class of bug.
Reworking the ownership is what breaks the pattern.

For the duration of a request, sock->file has no single owner.
Submit publishes the request without taking a file reference;
accept_doit acquires one inside the handler, after the request
has already left the pending list.  The consumer can drop its
own reference at any time, including the moment between
handshake_req_next() popping the request and accept_doit
reaching get_file().  The submit-side sock_hold() pins only
struct sock; struct socket and sock->file remain under the
consumer's control via the file descriptor.

This series places the file reference under unambiguous
ownership.  handshake_req_submit() pins it on the request and
completion or cancel drops it (patches 4-5); the submit-side
sock_hold() then becomes redundant, and dropping it also closes
a publish-before-pin race the late sock_hold itself opened
(patch 6).  The handshake_complete() API and its consumers move
to a uniform negative-errno sign convention (patch 3), with the
matching sign correction in nvme-tcp (patch 2).  Patch 1
hardens hn_lock for BH context, the netns-exit drain fix
builds on the new file-pin infrastructure (patch 8), and new
KUnit file-count assertions verify the refcount contract
(patch 7).

Three things in this restructuring want a careful look.  In
handshake_complete(), the fput() of the request's file
reference has to come after hp_done() -- fput() can transitively
run handshake_sk_destruct() and free the request, so the patch
stashes hr_file in a local first.  handshake_sk_destruct()
itself is kept on purpose: it owns rhashtable removal and
kfree, and remains the backstop if a consumer path bypasses
handshake_complete() entirely.  Third, handshake_req_next() now
returns its request with an extra get_file() held under
hn_lock; accept_doit must consume that reference (FD_PREPARE on
success, explicit fput on the fdf.err path), and any future
caller has to honor the same contract.

v2: https://patch.msgid.link/20260521-handshake-file-pin-v2-0-b9dadc472840@oracle.com
v1: https://patch.msgid.link/20260518-handshake-file-pin-v1-0-4bbcb7e62fda@oracle.com
====================

Link: https://patch.msgid.link/20260525-handshake-file-pin-v3-0-66c616906ead@oracle.com


Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parents 98d0912e ea5fe6a7

Documentation/netlink/specs/handshake.yaml

+8 −0
Original line number Diff line number Diff line
@@ -12,6 +12,12 @@ protocol: genetlink 
doc: Netlink protocol to request a transport layer security handshake.



definitions:

  -

    type: const

    name: max-errno

    value: 4095

    header: linux/err.h

    scope: kernel

  -

    type: enum

    name: handler-class
@@ -80,6 +86,8 @@ attribute-sets: 
      -

        name: status

        type: u32

        checks:

          max: max-errno

      -

        name: sockfd

        type: s32

drivers/nvme/host/tcp.c

+1 −1
Original line number Diff line number Diff line
@@ -1702,7 +1702,7 @@ static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid) 
		qid, pskid, status);



	if (status) {

		queue->tls_err = -status;

		queue->tls_err = status;

		goto out_complete;

	}

net/handshake/genl.c

+2 −1
Original line number Diff line number Diff line
@@ -10,6 +10,7 @@ 
#include "genl.h"



#include <uapi/linux/handshake.h>

#include <linux/err.h>



/* HANDSHAKE_CMD_ACCEPT - do */

static const struct nla_policy handshake_accept_nl_policy[HANDSHAKE_A_ACCEPT_HANDLER_CLASS + 1] = {
@@ -18,7 +19,7 @@ static const struct nla_policy handshake_accept_nl_policy[HANDSHAKE_A_ACCEPT_HAN 


/* HANDSHAKE_CMD_DONE - do */

static const struct nla_policy handshake_done_nl_policy[HANDSHAKE_A_DONE_REMOTE_AUTH + 1] = {

	[HANDSHAKE_A_DONE_STATUS] = { .type = NLA_U32, },

	[HANDSHAKE_A_DONE_STATUS] = NLA_POLICY_MAX(NLA_U32, MAX_ERRNO),

	[HANDSHAKE_A_DONE_SOCKFD] = { .type = NLA_S32, },

	[HANDSHAKE_A_DONE_REMOTE_AUTH] = { .type = NLA_U32, },

};

net/handshake/genl.h

+1 −0
Original line number Diff line number Diff line
@@ -11,6 +11,7 @@ 
#include <net/genetlink.h>



#include <uapi/linux/handshake.h>

#include <linux/err.h>



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);

net/handshake/handshake-test.c

+37 −1
Original line number Diff line number Diff line
@@ -25,7 +25,7 @@ static int test_accept_func(struct handshake_req *req, struct genl_info *info, 
	return 0;

}



static void test_done_func(struct handshake_req *req, unsigned int status,

static void test_done_func(struct handshake_req *req, int status,

			   struct genl_info *info)

{

}
@@ -208,6 +208,7 @@ static void handshake_req_submit_test3(struct kunit *test) 
static void handshake_req_submit_test4(struct kunit *test)

{

	struct handshake_req *req, *result;

	unsigned long fcount_before;

	struct socket *sock;

	struct file *filp;

	int err;
@@ -224,8 +225,10 @@ static void handshake_req_submit_test4(struct kunit *test) 
	KUNIT_ASSERT_NOT_NULL(test, sock->sk);

	sock->file = filp;



	fcount_before = file_count(filp);

	err = handshake_req_submit(sock, req, GFP_KERNEL);

	KUNIT_ASSERT_EQ(test, err, 0);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before + 1);



	/* Act */

	result = handshake_req_hash_lookup(sock->sk);
@@ -235,11 +238,13 @@ static void handshake_req_submit_test4(struct kunit *test) 
	KUNIT_EXPECT_PTR_EQ(test, req, result);



	handshake_req_cancel(sock->sk);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before);

	fput(filp);

}



static void handshake_req_submit_test5(struct kunit *test)

{

	unsigned long fcount_before;

	struct handshake_req *req;

	struct handshake_net *hn;

	struct socket *sock;
@@ -265,12 +270,14 @@ static void handshake_req_submit_test5(struct kunit *test) 


	saved = hn->hn_pending;

	hn->hn_pending = hn->hn_pending_max + 1;

	fcount_before = file_count(filp);



	/* Act */

	err = handshake_req_submit(sock, req, GFP_KERNEL);



	/* Assert */

	KUNIT_EXPECT_EQ(test, err, -EAGAIN);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before);



	fput(filp);

	hn->hn_pending = saved;
@@ -279,6 +286,7 @@ static void handshake_req_submit_test5(struct kunit *test) 
static void handshake_req_submit_test6(struct kunit *test)

{

	struct handshake_req *req1, *req2;

	unsigned long fcount_before;

	struct socket *sock;

	struct file *filp;

	int err;
@@ -296,21 +304,26 @@ static void handshake_req_submit_test6(struct kunit *test) 
	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);

	KUNIT_ASSERT_NOT_NULL(test, sock->sk);

	sock->file = filp;

	fcount_before = file_count(filp);



	/* Act */

	err = handshake_req_submit(sock, req1, GFP_KERNEL);

	KUNIT_ASSERT_EQ(test, err, 0);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before + 1);

	err = handshake_req_submit(sock, req2, GFP_KERNEL);



	/* Assert */

	KUNIT_EXPECT_EQ(test, err, -EBUSY);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before + 1);



	handshake_req_cancel(sock->sk);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before);

	fput(filp);

}



static void handshake_req_cancel_test1(struct kunit *test)

{

	unsigned long fcount_before;

	struct handshake_req *req;

	struct socket *sock;

	struct file *filp;
@@ -329,8 +342,10 @@ static void handshake_req_cancel_test1(struct kunit *test) 
	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);

	sock->file = filp;



	fcount_before = file_count(filp);

	err = handshake_req_submit(sock, req, GFP_KERNEL);

	KUNIT_ASSERT_EQ(test, err, 0);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before + 1);



	/* NB: handshake_req hasn't been accepted */
@@ -339,12 +354,14 @@ static void handshake_req_cancel_test1(struct kunit *test) 


	/* Assert */

	KUNIT_EXPECT_TRUE(test, result);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before);



	fput(filp);

}



static void handshake_req_cancel_test2(struct kunit *test)

{

	unsigned long fcount_before;

	struct handshake_req *req, *next;

	struct handshake_net *hn;

	struct socket *sock;
@@ -365,8 +382,10 @@ static void handshake_req_cancel_test2(struct kunit *test) 
	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);

	sock->file = filp;



	fcount_before = file_count(filp);

	err = handshake_req_submit(sock, req, GFP_KERNEL);

	KUNIT_ASSERT_EQ(test, err, 0);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before + 1);



	net = sock_net(sock->sk);

	hn = handshake_pernet(net);
@@ -375,18 +394,24 @@ static void handshake_req_cancel_test2(struct kunit *test) 
	/* Pretend to accept this request */

	next = handshake_req_next(hn, HANDSHAKE_HANDLER_CLASS_TLSHD);

	KUNIT_ASSERT_PTR_EQ(test, req, next);

	/* Simulate FD_PREPARE() consuming the file reference handed

	 * off by handshake_req_next(); see handshake_nl_accept_doit().

	 */

	fput(filp);



	/* Act */

	result = handshake_req_cancel(sock->sk);



	/* Assert */

	KUNIT_EXPECT_TRUE(test, result);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before);



	fput(filp);

}



static void handshake_req_cancel_test3(struct kunit *test)

{

	unsigned long fcount_before;

	struct handshake_req *req, *next;

	struct handshake_net *hn;

	struct socket *sock;
@@ -407,8 +432,10 @@ static void handshake_req_cancel_test3(struct kunit *test) 
	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);

	sock->file = filp;



	fcount_before = file_count(filp);

	err = handshake_req_submit(sock, req, GFP_KERNEL);

	KUNIT_ASSERT_EQ(test, err, 0);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before + 1);



	net = sock_net(sock->sk);

	hn = handshake_pernet(net);
@@ -417,15 +444,21 @@ static void handshake_req_cancel_test3(struct kunit *test) 
	/* Pretend to accept this request */

	next = handshake_req_next(hn, HANDSHAKE_HANDLER_CLASS_TLSHD);

	KUNIT_ASSERT_PTR_EQ(test, req, next);

	/* Simulate FD_PREPARE() consuming the file reference handed

	 * off by handshake_req_next(); see handshake_nl_accept_doit().

	 */

	fput(filp);



	/* Pretend to complete this request */

	handshake_complete(next, -ETIMEDOUT, NULL);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before);



	/* Act */

	result = handshake_req_cancel(sock->sk);



	/* Assert */

	KUNIT_EXPECT_FALSE(test, result);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before);



	fput(filp);

}
@@ -446,6 +479,7 @@ static struct handshake_proto handshake_req_alloc_proto_destroy = { 


static void handshake_req_destroy_test1(struct kunit *test)

{

	unsigned long fcount_before;

	struct handshake_req *req;

	struct socket *sock;

	struct file *filp;
@@ -465,10 +499,12 @@ static void handshake_req_destroy_test1(struct kunit *test) 
	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filp);

	sock->file = filp;



	fcount_before = file_count(filp);

	err = handshake_req_submit(sock, req, GFP_KERNEL);

	KUNIT_ASSERT_EQ(test, err, 0);



	handshake_req_cancel(sock->sk);

	KUNIT_EXPECT_EQ(test, file_count(filp), fcount_before);



	/* Act */

	/* Ensure the close/release/put process has run to