Re: [PATCH v3 3/3] net/hns3: fix Rx packet without CRC data

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 26 Nov 2024 10:40:27 +0800
huangdengdui <huangdengdui@huawei.com> wrote:

> On 2024/11/26 1:45, Stephen Hemminger wrote:
> > On Fri, 19 Jul 2024 17:04:15 +0800
> > Jie Hai <haijie1@huawei.com> wrote:
> >   
> >> +static inline void
> >> +hns3_recalculate_crc(struct rte_mbuf *m)
> >> +{
> >> +	char *append_data;
> >> +	uint32_t crc;
> >> +
> >> +	crc = rte_net_crc_calc(rte_pktmbuf_mtod(m, void *),
> >> +			       m->data_len, RTE_NET_CRC32_ETH);
> >> +
> >> +	/*
> >> +	 * The hns3 driver requires that mbuf size must be at least 512B.
> >> +	 * When CRC is stripped by hardware, the pkt_len must be less than
> >> +	 * or equal to 60B. Therefore, the space of the mbuf is enough
> >> +	 * to insert the CRC.
> >> +	 *
> >> +	 * In addition, after CRC is stripped by hardware, pkt_len and data_len
> >> +	 * do not contain the CRC length. Therefore, after CRC data is appended
> >> +	 * by PMD again, both pkt_len and data_len add the CRC length.
> >> +	 */
> >> +	append_data = rte_pktmbuf_append(m, RTE_NET_CRC32_ETH);
> >> +	/* The CRC data is binary data and does not care about the byte order. */
> >> +	rte_memcpy(append_data, (void *)&crc, RTE_NET_CRC32_ETH);
> >> +}
> >> +  
> > 
> > There is a bug here. If there is no space at end of mbuf (tailroom) then
> > rte_pktmbuf_append will return NULL. This would only happen if mbuf pool
> > was sized such that there was space of a full size packet without CRC
> > and then the code to add kept CRC was executed.
> > 
> > You need to check for NULL return and figure out some kind of unwind
> > path such as making it a receive error and dropping the packet.  
> 
> This situation has been described in the comments.
> The hns3 driver requires that mbuf size must be at least 512B.[1]
> When CRC needs to be recalculated, the packet length must be less than 60.
> So the space of the mbuf must be enough to insert the CRC.
> [1]:https://elixir.bootlin.com/dpdk/v23.11/source/drivers/net/hns3/hns3_rxtx.c#L1723

Ok, but maybe add an RTE_ASSERT() to be safe.