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

On 08/08/26 1:42 pm, Bryam Vargas wrote:
> Hidayath,
>
>> I have a standalone net-next patch that aborts the connection when
>> bytes_to_rcv + diff_prod exceeds rmb_desc->len.  The check sits before
>> the atomic_add(), so the accumulator is never written with an out-of-range
>> value
> That covers the follow-up I said I would send, and the placement is better than
> what I described: I had said a check after the atomic_add, which only notices
> the counter is already out of range. Yours doesn't let it get there. Consider my
> follow-up withdrawn -- I am not sending a competing patch.
>
> If it is useful for the Fixes decision: I ran the wrap++/count==0 vector on the
> real SMC-D path under KASAN while working on the cursor series. With only the
> per-cursor bound applied, bytes_to_rcv reaches 6*len and smc_rx_recvmsg() trips
> slab-out-of-bounds on a read of 5*len; each CDC advances exactly len, so
> diff == len and an advance-bound does not fire -- it's the accumulation that
> overruns, which is what your check catches. Logs on request if you want them in
> the commit message.
Hi Bryam,

Yes, please send the logs.  I would like to put the splat in the commit
message and credit you for the reproduction.  Right now my changelog only
talks about the accounting damage -- SIOCINQ showing a length that is not
there, and poll() staying readable with nothing to read.  A real
slab-out-of-bounds read is a much stronger claim.  Any format is fine, I
will trim it.

Your point that diff == len on every message, so an advance bound never
fires, is also worth putting in the changelog.  I had argued that only from
the arithmetic, not from a run.
>
> Two heads-up on collisions, since both are in flight this week rather than
> merged:
>
> smc_cdc_msg_recv_action() is also touched by "net/smc: order the CDC receive
> path against buffer publication" (v4, [email protected]),
> which hoists sndbuf_desc to the top of the function and gates the tx-trigger on
> it. Your hunk sits just above that gate, so whichever lands second will want a
> look rather than a blind rebase. I'd rather flag it now than after a conflict.
Agreed. From reading the code they are in different parts of the function
-- my hunk sits above the tx-trigger gate you add.

But my v2 also adds an out_of_sync check to
smcd_cdc_rx_tsklet(), and your v4 edits the same line:

   yours: keeps "if (!conn || conn->killed)" and adds the rmb_desc
          smp_load_acquire() after it
   mine:  rewrites it to "if (!conn || conn->killed || conn->out_of_sync)"

So they conflict.
>
> And you mentioned running the abort_work cancel for both transports in v2 --
> that edits smc_conn_free()'s SMC-D branch, which "net/smc: unregister the
> connection before draining the rx tasklet"
> ([email protected]) also rewrites: it drops
> the !list_empty guard around smc_ism_unset_conn(), moves the drain ahead of the
> detach, and clears conn->sndbuf_desc before freeing it. Same branch, same week.
Confirmed, that one conflicts too.  My v2 hunk moves
cancel_work_sync(&conn->abort_work) out of the non-SMC-D branch so it runs
for both transports.

So my v2 now conflicts with both of your patches.  Both of yours are posted
and mine is not, so I will rebase on top of both rather than ask you to
work around me.

If you would rather take the abort_work cancel into your
teardown 1/2 while you are already in that branch, please say so and I will
drop that hunk.
>
> On the shared bitfield -- agreed it needs a layout change rather than something
> folded into a fix, and it's yours; I'd noted it and left it alone for the
> same reason.
Thanks, I will send it separately.

One more thing, for information.  I have sent "net/smc: fix use-after-free
in smc_rx_pipe_buf_release()" to the list.  It clears conn->rmb_desc in
smc_buf_unuse(), which is the rmb version of the sndbuf_desc clear in your
teardown 1/2.  These are different functions, so from inspection they
should not conflict.

>
> Thanks,
> Bryam
Thanks,
Hidayath
>