Re: [PATCH net] xfrm: iptfs: do not seek the sequential skb reader backwards

[Steffen Klassert <steffen.klassert@secunet.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
commit d3c6d0500e742002c13ba4e7b9eff45bdb2c394b
Author: Jun Yang <junvyyang@tencent.com>
xfrm: iptfs: do not seek the sequential skb reader backwards

iptfs_reassem_cont() can underflow its offset arithmetic when reading
a saved runt if the sequential reader stepped, leading to an out-of-bounds
memcpy. This patch aborts and restarts the sequence read to avoid backwards
seeks. It also fixes a sibling parser to use offset 0 when restarting.

> diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
> index 6920940a35b49..751cceab738a1 100644
> --- a/net/xfrm/xfrm_iptfs.c
> +++ b/net/xfrm/xfrm_iptfs.c
[ ... ]
> @@ -1046,7 +1052,7 @@ static bool __input_process_payload(struct xfrm_state *x, u32 data,
>  			struct sk_buff *save = skbseq->root_skb;
>  
>  			skb_abort_seq_read(skbseq);
> -			skb_prepare_seq_read(save, data, tail, skbseq);
> +			skb_prepare_seq_read(save, 0, tail, skbseq);
>  		}
>  
>  		if (first_skb) {

Does this code introduce a data race that corrupts reassembly?

When __input_process_payload() saves a runt, it calls iptfs_input_save_runt()
without acquiring xtfs->drop_lock or any other SA-level lock:

net/xfrm/xfrm_iptfs.c:__input_process_payload() {
    ...
    /* save the bytes we have, advance data and exit */
    iptfs_input_save_runt(xtfs, seq, hbytes, remaining);
    ...
}

This locklessly modifies xtfs->ra_runt, xtfs->ra_runtlen, and
xtfs->ra_wantseq.

Concurrently, another CPU processing packets for the same IP-TFS SA might
execute iptfs_input_ordered() and read xtfs->ra_runtlen locklessly, then
acquire drop_lock for iptfs_reassem_cont():

net/xfrm/xfrm_iptfs.c:iptfs_input_ordered() {
    ...
    /* check before locking i.e., maybe */
    if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {
        spin_lock(&xtfs->drop_lock);

        /* check again after lock */
        if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {
            data = iptfs_reassem_cont(xtfs, seq, &skbseq, skb, data,
                                      blkoff, &sublist);
        }
        spin_unlock(&xtfs->drop_lock);
    }
    ...
}

Because iptfs_input_save_runt() does not hold drop_lock, the lockless read
and locked memcpy of xtfs->ra_runt race directly with the lockless writes.
Can this cause memory corruption, torn reads, and packet corruption?

Can this code cause a lock context violation on PREEMPT_RT kernels?

In iptfs_input_ordered(), a sleepable spinlock is acquired while a
kmap_atomic mapping might still be held:

net/xfrm/xfrm_iptfs.c:iptfs_input_ordered() {
    ...
    if (skb_copy_seq_read(&skbseq, 0, ipth, sizeof(*ipth))) {
    ...
    if (blkoff || xtfs->ra_runtlen || xtfs->ra_newskb) {
        spin_lock(&xtfs->drop_lock);
    ...
}

If the packet is non-linear and the header resides in a paged fragment,
skb_copy_seq_read() maps the fragment using kmap_atomic() and leaves
skbseq.frag_data actively mapped.

Acquiring spin_lock(&xtfs->drop_lock) without first aborting the sequence
read violates lock context rules, as spinlock_t maps to a sleepable rt_mutex
on PREEMPT_RT.

Could this lead to a 'sleeping in atomic context' bug?