Re: [PATCH net-next v3 4/8] ibmveth: step past bad RX correlators instead of spinning or oopsing

[mingming cao <mmc@linux.ibm.com>]

On 10/10/26 12:33 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 3 (1) · Medium: 0 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] This patch's short-frame check is incomplete.
> - [High] Returning budget - 1 does not make ibmveth_poll()'s schedule-
>    and-continue path safe when threaded NAPI and busy polling are both…
> - [High] net/ibmveth: TOCTOU double-fetch of RX correlator in
>    harvest_buffer
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>    ibmveth_rx_csum_helper() calls ip_fast_csum(iph, iph->ihl) whenever…
>
> --


Thank you for the review. I agree with all four High findings on this
patch.

I will address them in v4 on the same net-next base (d8674294aefe).
Patches 1-3 and 5-8 are unchanged from v3. Patch 4 fixes the poll
tail (no restart_poll after napi_complete_done; mask then queue the
NAPI), passes the correlator poll already read into harvest, and
keeps the buffer-bound and ETH_HLEN drops without claiming they cover
the RX offload helpers.

The helper header checks (short frames, ihl,
doff, fragments, and the ip_fast_csum() case) are split into a new
patch 9 at the end of the series, with their own Fixes: tags and KUnit
cases.

I will post v4 on netdev shortly.
v4 was tested with KUnit on qemu pseries and on a POWER10 LPAR (the v3
suite plus large-send iperf3 over IPv4/IPv6 at MTU 1500 and 9000).

Thanks,
Mingming

pw-bot: cr

> ibmveth: step past bad RX correlators instead of spinning or oopsing
>
> This patch makes ibmveth_poll() advance the RX ring past slots with a bad
> correlator, including correlators that name an inactive pool, instead of
> breaking out and restarting on the same slot. It counts those slots as
> drops and schedules a reset. It also drops frames that do not fit the pool
> buffer or are shorter than an Ethernet header, and returns budget - 1 once
> napi_schedule() has queued the NAPI.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index cee0e9783b2ab..55c0b5d6e0a9f 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -1481,19 +1525,46 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
> [ ... ]
>
>>   		} else {
>>   			struct sk_buff *skb, *new_skb;
>>   			int length = ibmveth_rxq_frame_length(adapter);
>>   			int offset = ibmveth_rxq_frame_offset(adapter);
>>   			int csum_good = ibmveth_rxq_csum_good(adapter);
>>   			int lrg_pkt = ibmveth_rxq_large_packet(adapter);
>> +			u64 correlator = ibmveth_rxq_correlator(adapter);
>> +			unsigned int pool, room, off, len;
>>   			__sum16 iph_check = 0;
> [Severity: High]
> The commit message says the bound uses the pool "from the correlator
> that was validated, read once, not from a second read of the ring".
> Doesn't ibmveth_rxq_harvest_buffer() still do that second read?
>
> ibmveth_poll() now reads the correlator once with
> ibmveth_rxq_correlator(). It uses that value to look up skb in
> ibmveth_rxq_get_buffer() and to compute room. Every harvest after that
> ignores it, though: this call, the copybreak call and the one on the
> bad-frame path. ibmveth_rxq_harvest_buffer() loads
> queue_addr[rx_queue.index].correlator again with a plain load and
> passes that to ibmveth_remove_buffer_from_pool(). Nothing ties the
> second value to the one poll validated.
>
> Suppose the entry changes between the two loads, so skb is looked up
> from slot A and harvested from slot B. On this reuse=false path:
>
> - rx_buff_pool[].skbuff[B] is cleared and dma_addr[B] is unmapped.
>    B goes on free_map, and the skb that was in slot B is leaked.
>
> - skb A goes to napi_gro_receive() while it is still in skbuff[A],
>    still DMA mapped and not on free_map. The driver still treats it
>    as a posted buffer.
>
> - On close or reset, ibmveth_free_buffer_pool() finds skb A in
>    skbuff[A], unmaps it and calls dev_kfree_skb_any() on it after the
>    stack has already freed it.
>
> The copybreak and bad-frame paths would recycle slot B instead of A
> in the same way.
>
> ibmveth_rxq_correlator_valid() does not catch this, because both
> correlators can be in range and name live skbs.
>
> PHYP should not rewrite an entry once the toggle is set. But this
> patch treats the correlator as untrusted, and the changelog says it
> is read once. Should ibmveth_rxq_harvest_buffer() take the correlator
> from its caller, so poll uses one value for lookup, bound check and
> harvest? The new KUnit harvest test could read the ring entry itself
> before calling it.
>
> [ ... ]
>
>> +			pool = correlator >> 32;
>> +			room = min_t(unsigned int, skb_tailroom(skb),
>> +				     adapter->rx_buff_pool[pool].buff_size);
>> +			off = offset;
>> +			len = length;
>> +			if (unlikely(len < ETH_HLEN || off >= room ||
>> +				     len > room - off)) {
>> +				if (net_ratelimit())
>> +					netdev_err(netdev,
>> +						   "bad RX frame offset %u length %u (buffer %u), dropping\n",
>> +						   off, len, room);
>> +				ibmveth_rxq_harvest_buffer(adapter, true);
>> +				netdev->stats.rx_dropped++;
>> +				frames_processed++;
>> +				continue;
>> +			}
> [Severity: High]
>
> The commit message says this check fixes the case where "the checksum
> helpers would write past a short frame". Is an ETH_HLEN minimum enough
> for that?
>
> Once this check passes, ibmveth_poll() and its helpers assume a full IP
> header and TCP header and never look at skb->len:
>
> ibmveth_poll()
>      iph_check = iph->check;
>      ibmveth_rx_mss_helper()     /* lrg_pkt or iph_check == 0xffff */
>          skb_shinfo(skb)->gso_size = ntohs(tcph->check);
>          tcph->check = 0;
>          hdr_len = offset + tcph->doff * 4;
>          skb_shinfo(skb)->gso_segs =
>                  DIV_ROUND_UP(skb->len - hdr_len, ...);
>      ibmveth_rx_csum_helper()    /* csum_good */
>          iph->check = ip_fast_csum((unsigned char *)iph, iph->ihl);
>          struct tcphdr *tcph = (struct tcphdr *)(skb->data + iphlen);
>          if (tcph->check == 0x0000) {
>              tcph->check = ~csum_tcpudp_magic(...);
>
> A frame of about 14 to 92 bytes passes the new check, but these helpers
> still read and write past the end of the frame. In
> ibmveth_rx_mss_helper(), skb->len - hdr_len can also wrap.
>
> On the copybreak path, those accesses land in the uninitialized tailroom
> of new_skb.
>
> On the non-copybreak path, they land in the pool buffer past the frame.
> That path runs when the GFP_ATOMIC copybreak allocation fails, or when
> rx_copybreak is set at or below the frame length.
>
> The new bound also accepts a short frame at the very end of the buffer.
> For pool 0 with room = 512, off = room - 34 and len = 34 leave
> skb->tail == skb->end after skb_put(). eth_type_trans() then leaves 20
> bytes of IPv4 header, so tcph sits at skb->end.
>
> Wouldn't the tcph->check writes then land in struct skb_shared_info
> (hwtstamps, gso_type, tskey or dataref, depending on ihl)?
> skb_partial_csum_set() rejects this only after the write has already
> happened.
>
> Should there be per-protocol skb->len checks, or pskb_may_pull(), before
> the IP and TCP headers are parsed, rather than a fixed minimum?
>
> [Severity: High]
>
> This isn't a bug introduced by this patch, but since this patch hardens
> the RX path: should ibmveth_rx_csum_helper() check iph->ihl before it
> calls ip_fast_csum()?
>
>      if (iph->check == 0xffff) {
>          iph->check = 0;
>          iph->check = ip_fast_csum((unsigned char *)iph,
>                                    iph->ihl);
>      }
>
> On ppc64, ip_fast_csum_nofold() in arch/powerpc/include/asm/checksum.h
> documents that ihl "is always >= 5" and does:
>
>      for (i = 0; i < ihl - 1; i++, ptr++)
>          s += *ptr;
>
> ihl is unsigned, so with ihl == 0, ihl - 1 wraps to 0xFFFFFFFF and the
> loop reads about 16 GiB starting at the RX buffer, in NAPI softirq
> context.
>
> The ihl, check and ethertype fields and the IBMVETH_RXQ_CSUM_GOOD flag
> all come from the sending partition. Could a single IPv4 frame with
> ihl = 0 and check = 0xffff from a peer on the same virtual LAN oops or
> stall this partition?
>
> [ ... ]
>
>> @@ -1581,10 +1661,16 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
>>   	if (ibmveth_rxq_pending_buffer(adapter) && napi_schedule(napi)) {
>>   		lpar_rc = h_vio_signal(adapter->vdev->unit_address,
>>   				       VIO_IRQ_DISABLE);
>> +		rescheduled = 1;
>>   		goto restart_poll;
>>   	}
>>   
>>   out:
>> +	/* napi_schedule() already queued us. Returning budget would
>> +	 * make busy_poll_stop() queue the napi a second time.
>> +	 */
>> +	if (rescheduled && budget && frames_processed >= budget)
>> +		return budget - 1;
>>   	return frames_processed;
>>   }
> [Severity: High]
>
> Does returning budget - 1 cover the case where threaded NAPI and busy
> polling are both enabled?
>
> busy_poll_stop() clears NAPI_STATE_IN_BUSY_POLL before it calls the
> driver poll. Its own comment says "Unless rc == budget we no longer own
> the NAPI instance". From there:
>
> busy_poll_stop()
>    ibmveth_poll()
>      napi_complete_done()             /* succeeds */
>      h_vio_signal(..., VIO_IRQ_ENABLE)
>      napi_schedule()
>        ____napi_schedule()
>          set_bit(NAPI_STATE_SCHED_THREADED, &napi->state);
>          wake_up_process(thread);
>      goto restart_poll;               /* keeps processing the ring */
>
> The woken kthread sees NAPI_STATE_SCHED_THREADED in napi_thread_wait()
> and calls __napi_poll()->ibmveth_poll() on another CPU. Meanwhile the
> original call is still running from restart_poll.
>
> Can the two pollers then race, with no lock, on rx_queue.index and
> toggle, rx_buff_pool[].skbuff[], free_map and producer_index? For
> example, one harvest with reuse=false could clear and unmap an skb while
> the other poller passes the same skb to napi_gro_receive().
>
> The budget - 1 return only runs after this overlap. Continuing to poll
> after napi_schedule() predates this patch, but this patch now reasons
> about NAPI ownership after napi_schedule(). Would it be better to stop
> processing once napi_schedule() succeeds, instead of jumping back to
> restart_poll?
>