On Mar 22, 2023, at 5:03 AM, Paolo Abeni [off-list ref] wrote:

On Tue, 2023-03-21 at 13:58 +0000, Chuck Lever III wrote:

quoted

quoted

On Mar 21, 2023, at 7:27 AM, Paolo Abeni [off-list ref] wrote:

On Sat, 2023-03-18 at 12:18 -0400, Chuck Lever wrote:

quoted

+/**
+ * handshake_req_alloc - consumer API to allocate a request
+ * @sock: open socket on which to perform a handshake
+ * @proto: security protocol
+ * @flags: memory allocation flags
+ *
+ * Returns an initialized handshake_req or NULL.
+ */
+struct handshake_req *handshake_req_alloc(struct socket *sock,
+					  const struct handshake_proto *proto,
+					  gfp_t flags)
+{
+	struct sock *sk = sock->sk;
+	struct net *net = sock_net(sk);
+	struct handshake_net *hn = handshake_pernet(net);
+	struct handshake_req *req;
+
+	if (!hn)
+		return NULL;
+
+	req = kzalloc(struct_size(req, hr_priv, proto->hp_privsize), flags);
+	if (!req)
+		return NULL;
+
+	sock_hold(sk);

The hr_sk reference counting is unclear to me. It looks like
handshake_req retain a reference to such socket, but
handshake_req_destroy()/handshake_sk_destruct() do not release it.

If we rely on sk_destruct to release the final reference count,
it will never get invoked.

quoted

Perhaps is better moving such sock_hold() into handshake_req_submit(),
once that the request is successful?

I will do that.

Personally, I find it more clear to bump a reference count when
saving a copy of the object's pointer, as is done in _alloc. But if
others find it easier the other way, I have no problem changing
it to suit community preferences.

I made the above suggestion because it looks like the sk reference is
not released in the handshake_req_submit() error path, but anything
addressing that would be good enough for me.

Indeed, that was a bug. I've avoided that by re-arranging things
as discussed.

[...]

quoted

quoted

quoted

+/**
+ * handshake_req_cancel - consumer API to cancel an in-progress handshake
+ * @sock: socket on which there is an ongoing handshake
+ *
+ * XXX: Perhaps killing the user space agent might also be necessary?
+ *
+ * Request cancellation races with request completion. To determine
+ * who won, callers examine the return value from this function.
+ *
+ * Return values:
+ *   %true - Uncompleted handshake request was canceled or not found
+ *   %false - Handshake request already completed
+ */
+bool handshake_req_cancel(struct socket *sock)
+{
+	struct handshake_req *req;
+	struct handshake_net *hn;
+	struct sock *sk;
+	struct net *net;
+
+	sk = sock->sk;
+	net = sock_net(sk);
+	req = handshake_req_hash_lookup(sk);
+	if (!req) {
+		trace_handshake_cancel_none(net, req, sk);
+		return true;
+	}
+
+	hn = handshake_pernet(net);
+	if (hn && remove_pending(hn, req)) {
+		/* Request hadn't been accepted */
+		trace_handshake_cancel(net, req, sk);
+		return true;
+	}
+	if (test_and_set_bit(HANDSHAKE_F_REQ_COMPLETED, &req->hr_flags)) {
+		/* Request already completed */
+		trace_handshake_cancel_busy(net, req, sk);
+		return false;
+	}
+
+	__sock_put(sk);

Same here.

I'll move the sock_hold() to _submit, and cook up a comment or two.

In such comments please also explain why sock_put() is not needed here
(and above). e.g. who is retaining the extra sk ref.

One assumes that the API consumer would have a reference, but
perhaps these call sites should be replaced with sock_put().

quoted

quoted

Side note, I think at this point some tests could surface here? If
user-space-based self-tests are too cumbersome and/or do not offer
adequate coverage perhaps you could consider using kunit?

I'm comfortable with Kunit, having just added a bunch of tests
for the kernel's SunRPC GSS Kerberos implementation.

There, however, I had clearly defined test cases to add, thanks
to the RFCs. I guess I'm a little unclear on what specific tests
would be necessary or valuable here. Suggestions and existing
examples are very welcome.

I *think* that a good start would be exercising the expected code
paths:

handshake_req_alloc, handshake_req_submit, handshake_complete
handshake_req_alloc, handshake_req_submit, handshake_cancel
or even
tls_*_hello_*, tls_handshake_accept, tls_handshake_done
tls_*_hello_*, tls_handshake_accept, tls_handshake_cancel

These aren't user APIs, not sure this kind of testing is
especially valuable. I'm thinking maybe the netlink
operations would be a better thing to unit-test, and that
might be better done with user space tests...?

plus explicitly triggering some errors path e.g. 

hn_pending_max+1 consecutive submit with no accept
handshake_cancel after handshake_complete
multiple handshake_complete on the same req
multiple handshake_cancel on the same req

OK. I'm wondering if a user agent needs to be running
for these, in which case, running Kunit in its stand-
alone mode (ie, under UML) might not work at all.

Just thinking out loud... Kunit after all might not be
the right tool for this job.


--
Chuck Lever