This series are minor bugfixes for rte_ethdev.c. Chengchang Tang (2): ethdev: fix data room size verification in Rx queue setup ethdev: fix VLAN offloads set if no relative capabilities lib/librte_ethdev/rte_ethdev.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) -- 2.7.4
From: Chengchang Tang <tangchengchang@huawei.com> In the rte_eth_rx_queue_setup API function, the local variable named mbp_buf_size, which is the data room size of the input parameter mp, is checked to guarantee that each memory chunck used for net device in the mbuf is bigger than the min_rx_bufsize. But if mbp_buf_size is less than RTE_PKTMBUF_HEADROOM, the value of the following statement will be a large number since the mbp_buf_size is a unsigned value. mbp_buf_size - RTE_PKTMBUF_HEADROOM As a result, it will cause a segment fault in this situation. This patch fixes it by adding a check condition to guarantee that the local varibale named mbp_buf_size is bigger than RTE_PKTMBUF_HEADROOM. Fixes: af75078fece3 ("first public release") Cc: stable@dpdk.org Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> --- lib/librte_ethdev/rte_ethdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 8e10a6f..122fd2a 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -1822,7 +1822,8 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id, } mbp_buf_size = rte_pktmbuf_data_room_size(mp); - if ((mbp_buf_size - RTE_PKTMBUF_HEADROOM) < dev_info.min_rx_bufsize) { + if (mbp_buf_size < RTE_PKTMBUF_HEADROOM || + (mbp_buf_size - RTE_PKTMBUF_HEADROOM) < dev_info.min_rx_bufsize) { RTE_ETHDEV_LOG(ERR, "%s mbuf_data_room_size %d < %d (RTE_PKTMBUF_HEADROOM=%d + min_rx_bufsize(dev)=%d)\n", mp->name, (int)mbp_buf_size, -- 2.7.4
From: Chengchang Tang <tangchengchang@huawei.com> Currently, there is a potential problem that calling the API function rte_eth_dev_set_vlan_offload to start a vlan hardware offloads which the driver does not support. if the PMD driver does not support the relative hardware offloads and does not check for it, the hardware setting will not change, but the relative offload in dev->data->dev_conf.rxmode.offloads will be turned on. It is supposed to check the hardware capabilities to decide whether the relative callback needs to be called just like the behavior in the API function named rte_eth_dev_configure. Fixes: 81f9db8ecc2c ("ethdev: add vlan offload support") Cc: stable@dpdk.org Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> --- lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index 122fd2a..5176a0e 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -3261,12 +3261,14 @@ rte_eth_dev_set_vlan_ether_type(uint16_t port_id, int rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) { + struct rte_eth_dev_info dev_info; struct rte_eth_dev *dev; int ret = 0; int mask = 0; int cur, org = 0; uint64_t orig_offloads; uint64_t dev_offloads; + uint64_t new_offloads; RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); dev = &rte_eth_devices[port_id]; @@ -3320,6 +3322,25 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) if (mask == 0) return ret; + ret = rte_eth_dev_info_get(port_id, &dev_info); + if (ret != 0) + return ret; + + /* + * New added Rx vlan offloading which are not enabled in + * rte_eth_dev_configure() must be within its device capabilities + */ + if ((dev_offloads & dev_info.rx_offload_capa) != dev_offloads) { + new_offloads = dev_offloads & ~orig_offloads; + RTE_ETHDEV_LOG(ERR, + "Ethdev port_id=%u requested new added vlan offloads " + "0x%" PRIx64 " must be within Rx offloads capabilities " + "0x%"PRIx64 " in %s()\n", + port_id, new_offloads, dev_info.rx_offload_capa, + __func__); + return -EINVAL; + } + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); dev->data->dev_conf.rxmode.offloads = dev_offloads; ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); -- 2.7.4
On 6/19/20 6:42 AM, Wei Hu (Xavier) wrote: > From: Chengchang Tang <tangchengchang@huawei.com> > > In the rte_eth_rx_queue_setup API function, the local variable named > mbp_buf_size, which is the data room size of the input parameter mp, > is checked to guarantee that each memory chunck used for net device > in the mbuf is bigger than the min_rx_bufsize. But if mbp_buf_size is > less than RTE_PKTMBUF_HEADROOM, the value of the following statement > will be a large number since the mbp_buf_size is a unsigned value. > mbp_buf_size - RTE_PKTMBUF_HEADROOM > As a result, it will cause a segment fault in this situation. > > This patch fixes it by adding a check condition to guarantee that the > local varibale named mbp_buf_size is bigger than RTE_PKTMBUF_HEADROOM. > > Fixes: af75078fece3 ("first public release") > Cc: stable@dpdk.org > > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> > Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> > --- > lib/librte_ethdev/rte_ethdev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 8e10a6f..122fd2a 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -1822,7 +1822,8 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id, > } > mbp_buf_size = rte_pktmbuf_data_room_size(mp); > > - if ((mbp_buf_size - RTE_PKTMBUF_HEADROOM) < dev_info.min_rx_bufsize) { > + if (mbp_buf_size < RTE_PKTMBUF_HEADROOM || > + (mbp_buf_size - RTE_PKTMBUF_HEADROOM) < dev_info.min_rx_bufsize) { May be it could be shorten to mbp_buf_size < dev_info.min_rx_bufsize + RTE_PKTMBUF_HEADROOM However, way suggested in the patch is 100% safe against integer overflow. > RTE_ETHDEV_LOG(ERR, > "%s mbuf_data_room_size %d < %d (RTE_PKTMBUF_HEADROOM=%d + min_rx_bufsize(dev)=%d)\n", > mp->name, (int)mbp_buf_size, > Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
On 6/19/20 6:42 AM, Wei Hu (Xavier) wrote: > From: Chengchang Tang <tangchengchang@huawei.com> > > Currently, there is a potential problem that calling the API function > rte_eth_dev_set_vlan_offload to start a vlan hardware offloads which the > driver does not support. if the PMD driver does not support the relative > hardware offloads and does not check for it, the hardware setting will > not change, but the relative offload in dev->data->dev_conf.rxmode.offloads > will be turned on. I'm sorry, but I don't understand what 'relative' means in the context. Also, please, use 'VLAN' instead of 'vlan' in the description and log message below. > It is supposed to check the hardware capabilities to decide whether the > relative callback needs to be called just like the behavior in the API > function named rte_eth_dev_configure. I think the direction of the fix is right, but it definitely duplicates checks which are done in some PMDs. I think that it would be good to do the cleanup in follow up patches. > Fixes: 81f9db8ecc2c ("ethdev: add vlan offload support") Most likely it is incorrect, since generic Rx offloads were introduced later. > Cc: stable@dpdk.org > > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> > Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> > --- > lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index 122fd2a..5176a0e 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -3261,12 +3261,14 @@ rte_eth_dev_set_vlan_ether_type(uint16_t port_id, > int > rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) > { > + struct rte_eth_dev_info dev_info; > struct rte_eth_dev *dev; > int ret = 0; > int mask = 0; > int cur, org = 0; > uint64_t orig_offloads; > uint64_t dev_offloads; > + uint64_t new_offloads; > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > dev = &rte_eth_devices[port_id]; > @@ -3320,6 +3322,25 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) > if (mask == 0) > return ret; > > + ret = rte_eth_dev_info_get(port_id, &dev_info); > + if (ret != 0) > + return ret; > + > + /* > + * New added Rx vlan offloading which are not enabled in > + * rte_eth_dev_configure() must be within its device capabilities > + */ > + if ((dev_offloads & dev_info.rx_offload_capa) != dev_offloads) { > + new_offloads = dev_offloads & ~orig_offloads; > + RTE_ETHDEV_LOG(ERR, > + "Ethdev port_id=%u requested new added vlan offloads " > + "0x%" PRIx64 " must be within Rx offloads capabilities " > + "0x%"PRIx64 " in %s()\n", > + port_id, new_offloads, dev_info.rx_offload_capa, > + __func__); > + return -EINVAL; > + } > + > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); > dev->data->dev_conf.rxmode.offloads = dev_offloads; > ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); >
Hi, Andrew Rybchenko On 2020/6/19 16:37, Andrew Rybchenko wrote: > On 6/19/20 6:42 AM, Wei Hu (Xavier) wrote: >> From: Chengchang Tang <tangchengchang@huawei.com> >> >> Currently, there is a potential problem that calling the API function >> rte_eth_dev_set_vlan_offload to start a vlan hardware offloads which the >> driver does not support. if the PMD driver does not support the relative >> hardware offloads and does not check for it, the hardware setting will >> not change, but the relative offload in dev->data->dev_conf.rxmode.offloads >> will be turned on. > I'm sorry, but I don't understand what 'relative' means in the > context. > > Also, please, use 'VLAN' instead of 'vlan' in the description > and log message below. OK, I will fix it in V2. >> It is supposed to check the hardware capabilities to decide whether the >> relative callback needs to be called just like the behavior in the API >> function named rte_eth_dev_configure. > I think the direction of the fix is right, but it definitely > duplicates checks which are done in some PMDs. I think that it > would be good to do the cleanup in follow up patches. I will clean them in V2. >> Fixes: 81f9db8ecc2c ("ethdev: add vlan offload support") > Most likely it is incorrect, since generic Rx offloads were > introduced later. OK, I will replace it with the following statement in V2. Fixes: a4996bd89c42 ("ethdev: new Rx/Tx offloads API") Thanks, Xavier > >> Cc: stable@dpdk.org >> >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> >> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> >> --- >> lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index 122fd2a..5176a0e 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -3261,12 +3261,14 @@ rte_eth_dev_set_vlan_ether_type(uint16_t port_id, >> int >> rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) >> { >> + struct rte_eth_dev_info dev_info; >> struct rte_eth_dev *dev; >> int ret = 0; >> int mask = 0; >> int cur, org = 0; >> uint64_t orig_offloads; >> uint64_t dev_offloads; >> + uint64_t new_offloads; >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> dev = &rte_eth_devices[port_id]; >> @@ -3320,6 +3322,25 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) >> if (mask == 0) >> return ret; >> >> + ret = rte_eth_dev_info_get(port_id, &dev_info); >> + if (ret != 0) >> + return ret; >> + >> + /* >> + * New added Rx vlan offloading which are not enabled in >> + * rte_eth_dev_configure() must be within its device capabilities >> + */ >> + if ((dev_offloads & dev_info.rx_offload_capa) != dev_offloads) { >> + new_offloads = dev_offloads & ~orig_offloads; >> + RTE_ETHDEV_LOG(ERR, >> + "Ethdev port_id=%u requested new added vlan offloads " >> + "0x%" PRIx64 " must be within Rx offloads capabilities " >> + "0x%"PRIx64 " in %s()\n", >> + port_id, new_offloads, dev_info.rx_offload_capa, >> + __func__); >> + return -EINVAL; >> + } >> + >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); >> dev->data->dev_conf.rxmode.offloads = dev_offloads; >> ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); >> >