[PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Hidayath Khan]

smc_cdc_msg_recv_action() accumulates the peer's producer cursor advance
into conn->bytes_to_rcv:

        atomic_add(diff_prod, &conn->bytes_to_rcv);
        /* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */

Nothing enforces that comment.  A conforming peer cannot advance its
producer cursor past our consumer cursor, so a peer that does fills the
accumulator without bound.

Bounding the cursor does not help: a wrap increment with count 0 is a legal
advance of exactly one bufferful, so every per-cursor check accepts it
while smc_curs_diff() returns size for each such message.

The copy-length clamps bound the copy but never repair the counter, so
SIOCINQ reports a readable length that does not exist and
smc_rx_data_available() keeps poll() readable forever with nothing for
recvmsg() to return.

Drop the connection instead, reusing the abort smc_cdc_msg_validate()
performs for an out-of-range sequence number.  The test precedes the
atomic_add() so the counter is never left inflated; abandoning the rest of
the message matches smc_cdc_msg_validate(), and abort_work tears the
connection down.

The test is written as a subtraction.  diff_prod is not bounded by the RMB:
smc_curs_diff() documents "difference cannot exceed size" as an assumption
about its inputs, but the peer controls the cursor it is computed from, so
it can return up to INT_MAX.  Adding that to bytes_to_rcv would overflow
and the comparison would then read as false, defeating the check in exactly
the case it exists for.  Subtracting cannot overflow, since bytes_to_rcv is
in [0, rmb_desc->len].

Both transports are affected: the accumulator is shared.

Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Reviewed-by: Mahanta Jambigi <mjambigi@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 net/smc/smc_cdc.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..d8f747bbc6f6 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -305,6 +305,27 @@ static void smc_cdc_handle_urg_data_arrival(struct smc_sock *smc,
 	sk_send_sigurg(&smc->sk);
 }
 
+/* Drop a connection whose peer violated the CDC protocol.  @link is the link
+ * the offending message arrived on, or NULL when it is not known to the caller
+ * (SMC-D, and the shared receive path).  When @link is NULL conn->lnk is left
+ * unchanged: for SMC-R it already points to the right QP; for SMC-D there is
+ * no per-link QP to abort on.
+ */
+static void smc_cdc_abort_conn(struct smc_sock *smc, struct smc_link *link)
+{
+	struct smc_connection *conn = &smc->conn;
+
+	conn->out_of_sync = 1;  /* prevent any further receives */
+	spin_lock_bh(&conn->send_lock);
+	conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
+	if (link)
+		conn->lnk = link;
+	spin_unlock_bh(&conn->send_lock);
+	sock_hold(&smc->sk); /* sock_put in abort_work */
+	if (!queue_work(smc_close_wq, &conn->abort_work))
+		sock_put(&smc->sk);
+}
+
 static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,
 				 struct smc_link *link)
 {
@@ -314,17 +335,8 @@ static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,
 
 	/* check that seqnum was seen before */
 	diff = conn->local_rx_ctrl.seqno - recv_seq;
-	if (diff < 0) { /* diff larger than 0x7fff */
-		/* drop connection */
-		conn->out_of_sync = 1;	/* prevent any further receives */
-		spin_lock_bh(&conn->send_lock);
-		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
-		conn->lnk = link;
-		spin_unlock_bh(&conn->send_lock);
-		sock_hold(&smc->sk); /* sock_put in abort_work */
-		if (!queue_work(smc_close_wq, &conn->abort_work))
-			sock_put(&smc->sk);
-	}
+	if (diff < 0) /* diff larger than 0x7fff */
+		smc_cdc_abort_conn(smc, link);  /* drop connection */
 }
 
 static void smc_cdc_msg_recv_action(struct smc_sock *smc,
@@ -376,6 +388,11 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
 				  &conn->local_rx_ctrl.prod);
 	if (diff_prod) {
+		if (diff_prod > conn->rmb_desc->len -
+				atomic_read(&conn->bytes_to_rcv)) {
+			smc_cdc_abort_conn(smc, NULL);
+			return;
+		}
 		if (conn->local_rx_ctrl.prod_flags.urg_data_present)
 			smc_cdc_handle_urg_data_arrival(smc, &diff_prod);
 		/* bytes_to_rcv is decreased in smc_recvmsg */

base-commit: cd138abf911f7a6b19656e446b09cff4453d1f3d
-- 
2.52.0

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804141109.542202-1-hidayath@linux.ibm.com?part=1
---
--- Patch [1]: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB ---
commit fc7927463c41bac1e3a598538d104b46770ee787
Author: Hidayath Khan <hidayath@linux.ibm.com>

net/smc: abort the connection when the peer overruns the RMB

The patch factors the connection-abort sequence out of
smc_cdc_msg_validate() into a new helper smc_cdc_abort_conn(), and calls it
from smc_cdc_msg_recv_action() when the peer's producer cursor advance would
push conn->bytes_to_rcv past conn->rmb_desc->len.  The test is placed before
the atomic_add() and the rest of the message is abandoned.

A few questions about the new helper and its new caller below.

[Severity: Low]
[Finding: main-1-0]
[Sources: opus-5, old-prompts]
The commit message describes a peer-triggerable, user-visible defect
(SIOCINQ reporting a readable length that does not exist, poll() staying
readable with nothing for recvmsg() to return), but the patch carries
neither a Fixes: tag nor Cc: stable.

The accumulation site being repaired:

	atomic_add(diff_prod, &conn->bytes_to_rcv);
	/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */

is part of the original CDC receive implementation, so every stable release
carrying net/smc looks affected.  Could a Fixes: tag be added so backporters
know which releases to pick this up for?

Simon says: In a similar vein, if the above is so then perhaps
            the patch should be targeted at net rather than net-next?

> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
> index 32d6d03df3214..d8f747bbc6f69 100644
> --- a/net/smc/smc_cdc.c
> +++ b/net/smc/smc_cdc.c
> @@ -305,6 +305,27 @@ static void smc_cdc_handle_urg_data_arrival(struct smc_sock *smc,
>  	sk_send_sigurg(&smc->sk);
>  }
>  
> +/* Drop a connection whose peer violated the CDC protocol.  @link is the link
> + * the offending message arrived on, or NULL when it is not known to the caller
> + * (SMC-D, and the shared receive path).  When @link is NULL conn->lnk is left
> + * unchanged: for SMC-R it already points to the right QP; for SMC-D there is
> + * no per-link QP to abort on.
> + */
> +static void smc_cdc_abort_conn(struct smc_sock *smc, struct smc_link *link)
> +{
> +	struct smc_connection *conn = &smc->conn;
> +
> +	conn->out_of_sync = 1;  /* prevent any further receives */

[Severity: Medium]
[Finding: main-2-6]
[Sources: opus-5, old-prompts]
The kerneldoc above states the link == NULL case covers SMC-D, and this
comment says the flag prevents any further receives.  Does
conn->out_of_sync actually stop receives on SMC-D?

The only reader of the flag is the SMC-R IB completion path:

net/smc/smc_cdc.c:smc_cdc_rx_handler() {
	...
	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
	if (!conn || conn->out_of_sync) {
		read_unlock_bh(&lgr->conns_lock);
		return;
	}
	...
}

The SMC-D entry point gates on a different flag:

net/smc/smc_cdc.c:smcd_cdc_rx_tsklet() {
	...
	if (!conn || conn->killed)
		return;

	data_cdc = (struct smcd_cdc_msg *)conn->rmb_desc->cpu_addr;
	...
}

and conn->killed is only set later, inside smc_conn_kill(), once abort_work
actually runs.

For the window in between, can every further ISM DMB interrupt re-enter
smc_cdc_msg_recv_action() with peer-controlled cursors, keep updating
conn->local_rx_ctrl via smc_cdc_msg_to_host() and conn->peer_rmbe_space, and
trip the new test again?  Since queue_work() only fails while the item is
still pending, would a second abort_work run smc_conn_kill() on an
already-killed connection once the first one has started?

Would also testing conn->out_of_sync in smcd_cdc_rx_tsklet(), or at the top
of smc_cdc_msg_recv_action(), address this?

[Severity: Medium]
[Finding: main-5-17]
[Sources: opus-5, old-prompts]
This store now happens on the transport-shared receive path in softirq
context.  The flags share one storage unit:

net/smc/smc.h:struct smc_connection {
	...
	u8			killed : 1;	/* abnormal termination */
	u8			freed : 1;	/* normal termination */
	u8			out_of_sync : 1; /* out of sync with peer */
};

so conn->out_of_sync = 1 is a read-modify-write of that byte.
smc_conn_kill(), running from the abort_work queued right below, writes
conn->killed = 1 into the same byte from process context under lock_sock().

The receive path does not defer to the backlog when the socket is owned:

net/smc/smc_cdc.c:smc_cdc_msg_recv() {
	sock_hold(&smc->sk);
	bh_lock_sock(&smc->sk);
	smc_cdc_msg_recv_action(smc, cdc);
	bh_unlock_sock(&smc->sk);
	sock_put(&smc->sk); /* no free sk in softirq-context */
}

Can the two writers interleave and lose the killed update:

tasklet:    load the flags byte (killed == 0)
abort_work: store killed = 1
tasklet:    store the byte back with out_of_sync = 1 and killed == 0

Since smcd_cdc_rx_tsklet() gates only on conn->killed, would a lost killed
bit let a newly scheduled tasklet keep dereferencing conn->rmb_desc->cpu_addr
after smc_conn_kill() has already run smc_ism_unset_conn() and
smcd_buf_detach()?  And on SMC-R, would a lost out_of_sync bit defeat the
receive barrier this patch relies on?

> +	spin_lock_bh(&conn->send_lock);
> +	conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
> +	if (link)
> +		conn->lnk = link;
> +	spin_unlock_bh(&conn->send_lock);
> +	sock_hold(&smc->sk); /* sock_put in abort_work */
> +	if (!queue_work(smc_close_wq, &conn->abort_work))
> +		sock_put(&smc->sk);
> +}

[Severity: High]
[Finding: main-3-9]
[Sources: opus-5, old-prompts]
This queues conn->abort_work with no is_smcd distinction, and the new caller
in smc_cdc_msg_recv_action() is reachable from smcd_cdc_rx_tsklet().  The
only other caller, smc_cdc_msg_validate(), runs from smc_cdc_rx_handler(),
an ib_wc handler, so is this the first path that can queue abort_work for an
SMC-D connection?

smc_conn_free() only cancels the work in its non-SMC-D branch:

net/smc/smc_core.c:smc_conn_free() {
	...
	if (lgr->is_smcd) {
		if (!list_empty(&lgr->list))
			smc_ism_unset_conn(conn);
		if (smc_ism_support_dmb_nocopy(lgr->smcd))
			smcd_buf_detach(conn);
		tasklet_kill(&conn->rx_tsklet);
	} else {
		smc_cdc_wait_pend_tx_wr(conn);
		if (current_work() != &conn->abort_work)
			cancel_work_sync(&conn->abort_work);
	}
	if (!list_empty(&lgr->list)) {
		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
		smc_lgr_unregister_conn(conn);
	}
	...
	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
}

So if the peer trips the new test and the application then closes the
socket, can a still-pending abort_work outlive smc_conn_free() and later
run smc_conn_kill()?

net/smc/smc_core.c:smc_conn_kill() {
	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
	...
	if (conn->lgr->is_smcd) {
		smc_ism_unset_conn(conn);
	...
}

Two things look problematic there.  conn->lgr is dereferenced
unconditionally although smc_conn_free() already dropped the connection's
reference with smc_lgr_put() and never NULLs conn->lgr.  And
smc_ism_unset_conn() uses a descriptor that smc_buf_unuse() already returned
to the reuse pool:

net/smc/smc_ism.c:smc_ism_unset_conn() {
	if (!conn->rmb_desc)
		return;

	spin_lock_irqsave(&conn->lgr->smcd->lock, flags);
	conn->lgr->smcd->conn[conn->rmb_desc->sba_idx] = NULL;
	spin_unlock_irqrestore(&conn->lgr->smcd->lock, flags);
}

Since smc_buf_get_slot() hands back any descriptor with used == 0 and the
new owner is registered via smc_ism_set_conn(), can this clear the
smcd->conn[] slot of a different, live connection and stop its receives?
The sock_hold() here keeps the smc_sock alive but does not pin the link
group or the buffer descriptor.

Simon says: I think the issue flagged below is out of scope for this patch.

[Severity: High]
[Finding: old-prompts-4-16]
[Sources: old-prompts]
This isn't a bug introduced by this patch, since smc_cdc_msg_validate() used
the same hold-and-queue idiom before, but the patch makes it reachable from
any peer-driven producer-cursor advance rather than only from
failover-validation messages, so it seems worth raising here.

The reference taken by sock_hold() is documented as released by abort_work,
and the only sock_put() is there:

net/smc/smc_core.c:smc_conn_abort_work() {
	...
	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */
}

But smc_conn_free() discards the return value when it cancels a still
pending item:

	if (current_work() != &conn->abort_work)
		cancel_work_sync(&conn->abort_work);

cancel_work_sync() returns true exactly when the work was removed without
ever executing.  In that case smc_conn_abort_work() never runs, so does the
struct smc_sock leak because sk_refcnt never reaches zero and sk_free() is
never called?

The sibling work item uses the compensating put:

net/smc/smc_close.c:smc_close_cancel_work() {
	...
	if (cancel_work_sync(&smc->conn.close_work))
		sock_put(sk);
	...
}

Should the abort_work cancel path do the same?

> @@ -314,17 +335,8 @@ static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,

[ ... ]

> @@ -376,6 +388,11 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>  	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
>  				  &conn->local_rx_ctrl.prod);
>  	if (diff_prod) {
> +		if (diff_prod > conn->rmb_desc->len -
> +				atomic_read(&conn->bytes_to_rcv)) {
> +			smc_cdc_abort_conn(smc, NULL);
> +			return;
> +		}

Simon says: Also, seems out of scope for this patch

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5, old-prompts]
This is a pre-existing issue, but two sibling accumulators in the same
function are fed by the same peer-controlled cursor and are still unbounded,
and both are updated before this new test runs:

	diff_cons = smc_curs_diff(conn->peer_rmbe_size, &cons_old,
				  &conn->local_rx_ctrl.cons);
	if (diff_cons) {
		...
		atomic_add(diff_cons, &conn->peer_rmbe_space);
		/* guarantee 0 <= peer_rmbe_space <= peer_rmbe_size */

and, on the SMC-D dmb-nocopy path:

			atomic_add(diff_tx, &conn->sndbuf_space);
			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */

Both deltas come from conn->local_rx_ctrl.cons, which is as peer-controlled
as the prod cursor this patch defends against, and smcd_cdc_msg_to_host()
copies it verbatim while smc_cdc_cursor_to_host() only rejects a backwards
move.

The commit message says:

    The test precedes the atomic_add() so the counter is never left inflated

Is that only true for bytes_to_rcv?  Even the message that trips the new
check has already inflated peer_rmbe_space (and sndbuf_space on
loopback-ISM) by the time the abort is queued.  Should the wording be
narrowed to bytes_to_rcv, or should the same treatment be applied to the
cons-derived accumulators?

>  		if (conn->local_rx_ctrl.prod_flags.urg_data_present)
>  			smc_cdc_handle_urg_data_arrival(smc, &diff_prod);

Simon says: Also, seems out of scope for this patch

[Severity: Critical]
[Finding: main-6-20]
[Sources: opus-5, old-prompts]
This isn't a bug introduced by this patch, but the new test bounds only the
delta and the commit message explicitly dismisses bounding the cursor
("Bounding the cursor does not help"), so it seems worth stating that the
delta check does not protect the call directly below it.

smc_cdc_handle_urg_data_arrival() uses the absolute peer-supplied count as a
byte offset into the RMB:

net/smc/smc_cdc.c:smc_cdc_handle_urg_data_arrival() {
	...
	base = (char *)conn->rmb_desc->cpu_addr + conn->rx_off;
	if (conn->urg_curs.count)
		conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);
	...
}

Can a peer keep prod.count out of range while still producing a small delta?
smc_curs_diff() computes the wrap case in unsigned int arithmetic:

net/smc/smc_cdc.h:smc_curs_diff() {
	if (old->wrap != new->wrap)
		return max_t(int, 0,
			     ((size - old->count) + new->count));

	return max_t(int, 0, (new->count - old->count));
}

With old = {wrap 0, count 0} and new = {wrap 1, count 0xfffffff0} this wraps
modulo 2^32 and yields len - 16, which passes

	diff_prod > conn->rmb_desc->len - atomic_read(&conn->bytes_to_rcv)

for small bytes_to_rcv, while prod.count stays near 4G.  A variant without
the wrap trick: a first message with count 0x80000000 gives
max_t(int, 0, negative) == 0 and is stored, then count 0x80000001 gives
diff_prod == 1.

Nothing clamps the absolute count: smc_cdc_cursor_to_host() only rejects a
backwards move, and smcd_cdc_msg_to_host() copies peer->prod.wrap and
peer->prod.count with no validation.  The byte read at that offset is then
handed to user space:

net/smc/smc_rx.c:smc_rx_recv_urg() {
	...
	rc = memcpy_to_msg(msg, &conn->urg_rx_byte, 1);
	...
}

so is this an out-of-bounds read at a peer-chosen offset that either leaks
kernel memory through recvmsg(MSG_OOB) or faults on unmapped memory?  Would
validating prod.count < conn->rmb_desc->len alongside the new delta test be
appropriate?

>  		/* bytes_to_rcv is decreased in smc_recvmsg */

Re: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

[Hidayathulla Khan I]

Thanks for the review.

On 05/08/26 9:33 pm, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260804141109.542202-1-hidayath@linux.ibm.com?part=1
> ---
> --- Patch [1]: [PATCH net-next] net/smc: abort the connection when the peer overruns the RMB ---
> commit fc7927463c41bac1e3a598538d104b46770ee787
> Author: Hidayath Khan <hidayath@linux.ibm.com>
>
> net/smc: abort the connection when the peer overruns the RMB
>
> The patch factors the connection-abort sequence out of
> smc_cdc_msg_validate() into a new helper smc_cdc_abort_conn(), and calls it
> from smc_cdc_msg_recv_action() when the peer's producer cursor advance would
> push conn->bytes_to_rcv past conn->rmb_desc->len.  The test is placed before
> the atomic_add() and the rest of the message is abandoned.
>
> A few questions about the new helper and its new caller below.
>
> [Severity: Low]
> [Finding: main-1-0]
> [Sources: opus-5, old-prompts]
> The commit message describes a peer-triggerable, user-visible defect
> (SIOCINQ reporting a readable length that does not exist, poll() staying
> readable with nothing for recvmsg() to return), but the patch carries
> neither a Fixes: tag nor Cc: stable.
>
> The accumulation site being repaired:
>
> 	atomic_add(diff_prod, &conn->bytes_to_rcv);
> 	/* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */
>
> is part of the original CDC receive implementation, so every stable release
> carrying net/smc looks affected.  Could a Fixes: tag be added so backporters
> know which releases to pick this up for?
>
> Simon says: In a similar vein, if the above is so then perhaps
>              the patch should be targeted at net rather than net-next?
Sure.  I will resend against net with

   Fixes: 5f08318f617b ("smc: connection data control (CDC)")
   Cc: stable@vger.kernel.org

I had chosen net-next because the patch changes behaviour, but the defect
is peer-triggerable and present in every release carrying net/smc, so net
is the better fit.
>
>> diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
>> index 32d6d03df3214..d8f747bbc6f69 100644
>> --- a/net/smc/smc_cdc.c
>> +++ b/net/smc/smc_cdc.c
>> @@ -305,6 +305,27 @@ static void smc_cdc_handle_urg_data_arrival(struct smc_sock *smc,
>>   	sk_send_sigurg(&smc->sk);
>>   }
>>   
>> +/* Drop a connection whose peer violated the CDC protocol.  @link is the link
>> + * the offending message arrived on, or NULL when it is not known to the caller
>> + * (SMC-D, and the shared receive path).  When @link is NULL conn->lnk is left
>> + * unchanged: for SMC-R it already points to the right QP; for SMC-D there is
>> + * no per-link QP to abort on.
>> + */
>> +static void smc_cdc_abort_conn(struct smc_sock *smc, struct smc_link *link)
>> +{
>> +	struct smc_connection *conn = &smc->conn;
>> +
>> +	conn->out_of_sync = 1;  /* prevent any further receives */
> [Severity: Medium]
> [Finding: main-2-6]
> [Sources: opus-5, old-prompts]
> The kerneldoc above states the link == NULL case covers SMC-D, and this
> comment says the flag prevents any further receives.  Does
> conn->out_of_sync actually stop receives on SMC-D?
No, it does not.  Mine is the first code that sets the flag on an SMC-D
connection, and smcd_cdc_rx_tsklet() gates only on killed.  I will test
out_of_sync there as well in v2.
>
> The only reader of the flag is the SMC-R IB completion path:
>
> net/smc/smc_cdc.c:smc_cdc_rx_handler() {
> 	...
> 	conn = smc_lgr_find_conn(ntohl(cdc->token), lgr);
> 	if (!conn || conn->out_of_sync) {
> 		read_unlock_bh(&lgr->conns_lock);
> 		return;
> 	}
> 	...
> }
>
> The SMC-D entry point gates on a different flag:
>
> net/smc/smc_cdc.c:smcd_cdc_rx_tsklet() {
> 	...
> 	if (!conn || conn->killed)
> 		return;
>
> 	data_cdc = (struct smcd_cdc_msg *)conn->rmb_desc->cpu_addr;
> 	...
> }
>
> and conn->killed is only set later, inside smc_conn_kill(), once abort_work
> actually runs.
>
> For the window in between, can every further ISM DMB interrupt re-enter
> smc_cdc_msg_recv_action() with peer-controlled cursors, keep updating
> conn->local_rx_ctrl via smc_cdc_msg_to_host() and conn->peer_rmbe_space, and
> trip the new test again?  Since queue_work() only fails while the item is
> still pending, would a second abort_work run smc_conn_kill() on an
> already-killed connection once the first one has started?
>
> Would also testing conn->out_of_sync in smcd_cdc_rx_tsklet(), or at the top
> of smc_cdc_msg_recv_action(), address this?
>
> [Severity: Medium]
> [Finding: main-5-17]
> [Sources: opus-5, old-prompts]
> This store now happens on the transport-shared receive path in softirq
> context.  The flags share one storage unit:
>
> net/smc/smc.h:struct smc_connection {
> 	...
> 	u8			killed : 1;	/* abnormal termination */
> 	u8			freed : 1;	/* normal termination */
> 	u8			out_of_sync : 1; /* out of sync with peer */
> };
>
> so conn->out_of_sync = 1 is a read-modify-write of that byte.
> smc_conn_kill(), running from the abort_work queued right below, writes
> conn->killed = 1 into the same byte from process context under lock_sock().
>
> The receive path does not defer to the backlog when the socket is owned:
>
> net/smc/smc_cdc.c:smc_cdc_msg_recv() {
> 	sock_hold(&smc->sk);
> 	bh_lock_sock(&smc->sk);
> 	smc_cdc_msg_recv_action(smc, cdc);
> 	bh_unlock_sock(&smc->sk);
> 	sock_put(&smc->sk); /* no free sk in softirq-context */
> }
>
> Can the two writers interleave and lose the killed update:
Yes, the three flags share a byte.  Nobody appears to be working on it,
and it needs a layout change rather than a fix folded in here.  I will
send it separately.
>
> tasklet:    load the flags byte (killed == 0)
> abort_work: store killed = 1
> tasklet:    store the byte back with out_of_sync = 1 and killed == 0
>
> Since smcd_cdc_rx_tsklet() gates only on conn->killed, would a lost killed
> bit let a newly scheduled tasklet keep dereferencing conn->rmb_desc->cpu_addr
> after smc_conn_kill() has already run smc_ism_unset_conn() and
> smcd_buf_detach()?  And on SMC-R, would a lost out_of_sync bit defeat the
> receive barrier this patch relies on?
>
>> +	spin_lock_bh(&conn->send_lock);
>> +	conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
>> +	if (link)
>> +		conn->lnk = link;
>> +	spin_unlock_bh(&conn->send_lock);
>> +	sock_hold(&smc->sk); /* sock_put in abort_work */
>> +	if (!queue_work(smc_close_wq, &conn->abort_work))
>> +		sock_put(&smc->sk);
>> +}
> [Severity: High]
> [Finding: main-3-9]
> [Sources: opus-5, old-prompts]
> This queues conn->abort_work with no is_smcd distinction, and the new caller
> in smc_cdc_msg_recv_action() is reachable from smcd_cdc_rx_tsklet().  The
> only other caller, smc_cdc_msg_validate(), runs from smc_cdc_rx_handler(),
> an ib_wc handler, so is this the first path that can queue abort_work for an
> SMC-D connection?
>
> smc_conn_free() only cancels the work in its non-SMC-D branch:
>
> net/smc/smc_core.c:smc_conn_free() {
> 	...
> 	if (lgr->is_smcd) {
> 		if (!list_empty(&lgr->list))
> 			smc_ism_unset_conn(conn);
> 		if (smc_ism_support_dmb_nocopy(lgr->smcd))
> 			smcd_buf_detach(conn);
> 		tasklet_kill(&conn->rx_tsklet);
> 	} else {
> 		smc_cdc_wait_pend_tx_wr(conn);
> 		if (current_work() != &conn->abort_work)
> 			cancel_work_sync(&conn->abort_work);
> 	}
> 	if (!list_empty(&lgr->list)) {
> 		smc_buf_unuse(conn, lgr); /* allow buffer reuse */
> 		smc_lgr_unregister_conn(conn);
> 	}
> 	...
> 	smc_lgr_put(lgr); /* lgr_hold in smc_conn_create() */
> }
>
> So if the peer trips the new test and the application then closes the
> socket, can a still-pending abort_work outlive smc_conn_free() and later
> run smc_conn_kill()?
Yes.  Same root cause: smc_conn_free() cancels abort_work only in its
non-SMC-D branch, which was enough while smc_cdc_msg_validate() was the
only queuer.  I have confirmed both consequences you list, the stale
conn->lgr, and the smcd->conn[] slot of a descriptor already handed to
another connection.  I will run the cancel for both transports in v2.
>
> net/smc/smc_core.c:smc_conn_kill() {
> 	if (conn->lgr->is_smcd && conn->lgr->peer_shutdown)
> 	...
> 	if (conn->lgr->is_smcd) {
> 		smc_ism_unset_conn(conn);
> 	...
> }
>
> Two things look problematic there.  conn->lgr is dereferenced
> unconditionally although smc_conn_free() already dropped the connection's
> reference with smc_lgr_put() and never NULLs conn->lgr.  And
> smc_ism_unset_conn() uses a descriptor that smc_buf_unuse() already returned
> to the reuse pool:
>
> net/smc/smc_ism.c:smc_ism_unset_conn() {
> 	if (!conn->rmb_desc)
> 		return;
>
> 	spin_lock_irqsave(&conn->lgr->smcd->lock, flags);
> 	conn->lgr->smcd->conn[conn->rmb_desc->sba_idx] = NULL;
> 	spin_unlock_irqrestore(&conn->lgr->smcd->lock, flags);
> }
>
> Since smc_buf_get_slot() hands back any descriptor with used == 0 and the
> new owner is registered via smc_ism_set_conn(), can this clear the
> smcd->conn[] slot of a different, live connection and stop its receives?
> The sock_hold() here keeps the smc_sock alive but does not pin the link
> group or the buffer descriptor.
>
> Simon says: I think the issue flagged below is out of scope for this patch.
>
> [Severity: High]
> [Finding: old-prompts-4-16]
> [Sources: old-prompts]
> This isn't a bug introduced by this patch, since smc_cdc_msg_validate() used
> the same hold-and-queue idiom before, but the patch makes it reachable from
> any peer-driven producer-cursor advance rather than only from
> failover-validation messages, so it seems worth raising here.
>
> The reference taken by sock_hold() is documented as released by abort_work,
> and the only sock_put() is there:
>
> net/smc/smc_core.c:smc_conn_abort_work() {
> 	...
> 	sock_put(&smc->sk); /* sock_hold done by schedulers of abort_work */
> }
>
> But smc_conn_free() discards the return value when it cancels a still
> pending item:
>
> 	if (current_work() != &conn->abort_work)
> 		cancel_work_sync(&conn->abort_work);
>
> cancel_work_sync() returns true exactly when the work was removed without
> ever executing.  In that case smc_conn_abort_work() never runs, so does the
> struct smc_sock leak because sk_refcnt never reaches zero and sk_free() is
> never called?
>
> The sibling work item uses the compensating put:
>
> net/smc/smc_close.c:smc_close_cancel_work() {
> 	...
> 	if (cancel_work_sync(&smc->conn.close_work))
> 		sock_put(sk);
> 	...
> }
>
> Should the abort_work cancel path do the same?
Agreed. It is a real leak though, and I have already
sent a patch for it: "net/smc: drop the abort_work reference when the
work is cancelled"  which adds the compensating sock_put() using the same
smc_close_cancel_work() idiom that the finding points to.
>
>> @@ -314,17 +335,8 @@ static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,
> [ ... ]
>
>> @@ -376,6 +388,11 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
>>   	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
>>   				  &conn->local_rx_ctrl.prod);
>>   	if (diff_prod) {
>> +		if (diff_prod > conn->rmb_desc->len -
>> +				atomic_read(&conn->bytes_to_rcv)) {
>> +			smc_cdc_abort_conn(smc, NULL);
>> +			return;
>> +		}
> Simon says: Also, seems out of scope for this patch
>
> [Severity: Medium]
> [Finding: main-1-1]
> [Sources: opus-5, old-prompts]
> This is a pre-existing issue, but two sibling accumulators in the same
> function are fed by the same peer-controlled cursor and are still unbounded,
> and both are updated before this new test runs:
>
> 	diff_cons = smc_curs_diff(conn->peer_rmbe_size, &cons_old,
> 				  &conn->local_rx_ctrl.cons);
> 	if (diff_cons) {
> 		...
> 		atomic_add(diff_cons, &conn->peer_rmbe_space);
> 		/* guarantee 0 <= peer_rmbe_space <= peer_rmbe_size */
>
> and, on the SMC-D dmb-nocopy path:
>
> 			atomic_add(diff_tx, &conn->sndbuf_space);
> 			/* guarantee 0 <= sndbuf_space <= sndbuf_desc->len */
>
> Both deltas come from conn->local_rx_ctrl.cons, which is as peer-controlled
> as the prod cursor this patch defends against, and smcd_cdc_msg_to_host()
> copies it verbatim while smc_cdc_cursor_to_host() only rejects a backwards
> move.
>
> The commit message says:
>
>      The test precedes the atomic_add() so the counter is never left inflated
>
> Is that only true for bytes_to_rcv?
Yes, only bytes_to_rcv.  peer_rmbe_space and sndbuf_space are already
moved by the time the abort is queued.  I will narrow the wording in v2.
Bounding the cons-derived accumulators is a separate
change, and "net/smc: bound the send length to the send buffer in
smc_tx_sendmsg()" already clamps the sndbuf_space one where it is consumed.
>    Even the message that trips the new
> check has already inflated peer_rmbe_space (and sndbuf_space on
> loopback-ISM) by the time the abort is queued.  Should the wording be
> narrowed to bytes_to_rcv, or should the same treatment be applied to the
> cons-derived accumulators?
>
>>   		if (conn->local_rx_ctrl.prod_flags.urg_data_present)
>>   			smc_cdc_handle_urg_data_arrival(smc, &diff_prod);
> Simon says: Also, seems out of scope for this patch
>
> [Severity: Critical]
> [Finding: main-6-20]
> [Sources: opus-5, old-prompts]
> This isn't a bug introduced by this patch, but the new test bounds only the
> delta and the commit message explicitly dismisses bounding the cursor
> ("Bounding the cursor does not help"), so it seems worth stating that the
> delta check does not protect the call directly below it.
>
> smc_cdc_handle_urg_data_arrival() uses the absolute peer-supplied count as a
> byte offset into the RMB:
>
> net/smc/smc_cdc.c:smc_cdc_handle_urg_data_arrival() {
> 	...
> 	base = (char *)conn->rmb_desc->cpu_addr + conn->rx_off;
> 	if (conn->urg_curs.count)
> 		conn->urg_rx_byte = *(base + conn->urg_curs.count - 1);
> 	...
> }
>
> Can a peer keep prod.count out of range while still producing a small delta?
> smc_curs_diff() computes the wrap case in unsigned int arithmetic:
>
> net/smc/smc_cdc.h:smc_curs_diff() {
> 	if (old->wrap != new->wrap)
> 		return max_t(int, 0,
> 			     ((size - old->count) + new->count));
>
> 	return max_t(int, 0, (new->count - old->count));
> }
>
> With old = {wrap 0, count 0} and new = {wrap 1, count 0xfffffff0} this wraps
> modulo 2^32 and yields len - 16, which passes
>
> 	diff_prod > conn->rmb_desc->len - atomic_read(&conn->bytes_to_rcv)
>
> for small bytes_to_rcv, while prod.count stays near 4G.  A variant without
> the wrap trick: a first message with count 0x80000000 gives
> max_t(int, 0, negative) == 0 and is stored, then count 0x80000001 gives
> diff_prod == 1.
>
> Nothing clamps the absolute count: smc_cdc_cursor_to_host() only rejects a
> backwards move, and smcd_cdc_msg_to_host() copies peer->prod.wrap and
> peer->prod.count with no validation.  The byte read at that offset is then
> handed to user space:
>
> net/smc/smc_rx.c:smc_rx_recv_urg() {
> 	...
> 	rc = memcpy_to_msg(msg, &conn->urg_rx_byte, 1);
> 	...
> }
>
> so is this an out-of-bounds read at a peer-chosen offset that either leaks
> kernel memory through recvmsg(MSG_OOB) or faults on unmapped memory?  Would
> validating prod.count < conn->rmb_desc->len alongside the new delta test be
> appropriate?
The analysis is right, but "net/smc: bound the wire-controlled producer
cursor to the RMB" already clamps prod.count at the conversion, which
seems the better place for it.  I am happy to reorder behind that series.

Thanks again.

>
>>   		/* bytes_to_rcv is decreased in smc_recvmsg */