| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<SJ0PR18MB5216FB752575EE9CD64225ECDB122@SJ0PR18MB5216.namprd18.prod.outlook.com>
Date: Wed, 8 Jan 2025 14:02:44 +0000
From: Suman Ghosh <sumang@...vell.com>
To: Simon Horman <horms@...nel.org>
CC: Sunil Kovvuri Goutham <sgoutham@...vell.com>,
Geethasowjanya Akula
<gakula@...vell.com>,
Subbaraya Sundeep Bhatta <sbhatta@...vell.com>,
Hariprasad Kelam <hkelam@...vell.com>,
"davem@...emloft.net"
<davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Linu Cherian
<lcherian@...vell.com>, Jerin Jacob <jerinj@...vell.com>
Subject: RE: [EXTERNAL] Re: [net-next PATCH 2/6] octeontx2-pf: AF_XDP zero
copy receive support
Hi Simon,
>> @@ void otx2_free_aura_ptr(struct otx2_nic *pfvf, int type)
>>
>> /* Free SQB and RQB pointers from the aura pool */
>> for (pool_id = pool_start; pool_id < pool_end; pool_id++) {
>> - iova = otx2_aura_allocptr(pfvf, pool_id);
>> pool = &pfvf->qset.pool[pool_id];
>> + iova = otx2_aura_allocptr(pfvf, pool_id);
>> while (iova) {
>> if (type == AURA_NIX_RQ)
>> iova -= OTX2_HEAD_ROOM;
>> @@ -1323,6 +1337,13 @@ void otx2_free_aura_ptr(struct otx2_nic *pfvf,
>int type)
>> iova = otx2_aura_allocptr(pfvf, pool_id);
>> }
>> }
>> +
>> + for (idx = 0 ; idx < pool->xdp_cnt; idx++) {
>> + if (!pool->xdp[idx])
>> + continue;
>> +
>> + xsk_buff_free(pool->xdp[idx]);
>> + }
>
>Looking over otx2_pool_init(), I am wondering if the loop above run
>should over all (non AURA_NIX_RQ) pools, rather than the last pool
>covered by the preceding loop.
[Suman] Yes, you are right. Thanks for catching this, will update in v2.
>
>This was flagged by Smatch, because it is concerned that pool may be
>used unset above, presumably if the preceding loop iterates zero times
>(I'm unsure if that can happen in practice).
[Suman] Will update the logic in v2.
>
>...
>
>> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
>> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_pf.c
>
>...
>
>> @@ -3204,6 +3199,10 @@ static int otx2_probe(struct pci_dev *pdev,
>const struct pci_device_id *id)
>> /* Enable link notifications */
>> otx2_cgx_config_linkevents(pf, true);
>>
>> + pf->af_xdp_zc_qidx = bitmap_zalloc(qcount, GFP_KERNEL);
>> + if (!pf->af_xdp_zc_qidx)
>> + goto err_pf_sriov_init;
>> +
>
>This goto will result in the function returning err.
>However, here err is 0. Should it be set to a negative error value
>instead?
[Suman] Yes, will update in v2
>
>> #ifdef CONFIG_DCB
>> err = otx2_dcbnl_set_ops(netdev);
>> if (err)
>> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
>
>...
>
>> @@ -571,6 +574,7 @@ int otx2_napi_handler(struct napi_struct *napi,
>int budget)
>> if (pfvf->flags & OTX2_FLAG_ADPTV_INT_COAL_ENABLED)
>> otx2_adjust_adaptive_coalese(pfvf, cq_poll);
>>
>> + pool = &pfvf->qset.pool[cq->cq_idx];
>
>I am also unsure if this can happen in practice, but Smatch is concerned
>that cq may be used uninitialised here. It does seem that could
>theoretically be the case if, in the for loop towards the top of this
>function, cq_poll->cq_ids[i] is always CINT_INVALID_CQ.
>
>> if (unlikely(!filled_cnt)) {
>> struct refill_work *work;
>> struct delayed_work *dwork;
>
>...
>
>> @@ -1426,13 +1440,24 @@ static bool otx2_xdp_rcv_pkt_handler(struct
>otx2_nic *pfvf,
>> unsigned char *hard_start;
>> struct otx2_pool *pool;
>> int qidx = cq->cq_idx;
>> - struct xdp_buff xdp;
>> + struct xdp_buff xdp, *xsk_buff = NULL;
>> struct page *page;
>> u64 iova, pa;
>> u32 act;
>> int err;
>>
>> pool = &pfvf->qset.pool[qidx];
>> +
>> + if (pool->xsk_pool) {
>> + xsk_buff = pool->xdp[--cq->rbpool->xdp_top];
>> + if (!xsk_buff)
>> + return false;
>> +
>> + xsk_buff->data_end = xsk_buff->data + cqe->sg.seg_size;
>> + act = bpf_prog_run_xdp(prog, xsk_buff);
>> + goto handle_xdp_verdict;
>
>iova is not initialised until a few lines further down, which does not
>occur in the case of this condition.
[Suman] I think the patch ordering is wrong.
"patch#4: octeontx2-pf: Don't unmap page pool buffer used by XDP" is fixing this. I will update the patch order.
>
>> + }
>> +
>> iova = cqe->sg.seg_addr - OTX2_HEAD_ROOM;
>> pa = otx2_iova_to_phys(pfvf->iommu_domain, iova);
>> page = virt_to_page(phys_to_virt(pa)); @@ -1445,6 +1470,7 @@ static
>> bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
>>
>> act = bpf_prog_run_xdp(prog, &xdp);
>>
>> +handle_xdp_verdict:
>> switch (act) {
>> case XDP_PASS:
>> break;
>
>The lines lines of this function, between the hunk above and the one
>below looks like this:
[Suman] replied above.
>
> case XDP_TX:
> qidx += pfvf->hw.tx_queues;
> cq->pool_ptrs++;
> return otx2_xdp_sq_append_pkt(pfvf, iova,
>
>The above uses iova, but in the case that we got here via goto
>handle_xdp_verdict it is uninitialised.
[Suman] same as above.
>
>...
>
>> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> index e926c6ce96cf..9bb7e5c3e227 100644
>> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c
>> @@ -722,6 +722,10 @@ static int otx2vf_probe(struct pci_dev *pdev,
>const struct pci_device_id *id)
>> if (err)
>> goto err_shutdown_tc;
>>
>> + vf->af_xdp_zc_qidx = bitmap_zalloc(qcount, GFP_KERNEL);
>> + if (!vf->af_xdp_zc_qidx)
>> + goto err_shutdown_tc;
>
>Along the same lines of my comment on otx2_probe():
>should err be set to a negative error value here?
[Suman] yes, will update in v2
>
>...
>
>--
>pw-bot: changes-requested
Powered by blists - more mailing lists