From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by smtp.lore.kernel.org (Postfix) with ESMTP id F0971D38FF2 for ; Wed, 14 Jan 2026 17:14:06 +0000 (UTC) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 45EA640A72; Wed, 14 Jan 2026 18:14:06 +0100 (CET) Received: from mail-ej1-f65.google.com (mail-ej1-f65.google.com [209.85.218.65]) by mails.dpdk.org (Postfix) with ESMTP id E9DC640A70 for ; Wed, 14 Jan 2026 18:14:04 +0100 (CET) Received: by mail-ej1-f65.google.com with SMTP id a640c23a62f3a-b86f69bbe60so11680066b.1 for ; Wed, 14 Jan 2026 09:14:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1768410844; x=1769015644; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=tpislOFBGg3o0LWmxGUaJxTAfWiOmTCXKo5SuVFeSLE=; b=DtN4ecBzhWl6wTV4vnN9KBfFQ1ONobUeU2/IY9ZBxmTJexPBhGgw8UQgs7sxUJrIWs WDVBixdlZNjJXi4nq7BKaPWMLNRyJeYcZ4oS4F/mVRPybQFJ9hNIX7X2DkkeiC7PSQb9 lrxtCUhVV163oXMCwCfXrXakhIWo5A51VC5skRD1cqS6HsFyawRtOsH1nu66oXmmhSh+ /uPFHaywXVaclFpmo4+gbZ3irFV+wFj4Xhw+hYSurpsAGdS1eBYk64NgaCFd5PmnIUN+ bI4JKPUOrP6ATE5wmin/oSfwTGDDhwRo4+EoyD3J4SwhrtFYhOrGl1C+a/xhFUus71ZD MZtw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768410844; x=1769015644; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=tpislOFBGg3o0LWmxGUaJxTAfWiOmTCXKo5SuVFeSLE=; b=S4OPxnadZlO9b0YbzPhUvXiXGJQnxuft3oqLSvzDQyldW+atZ6gpsaSia2Qfb6GcF2 YC86oAHBNpucpsKOfCY1xZuUs06oS5fou+u9Tj+T/CIrT8Fo+uYXyGG3XoQyzu1X4ve3 rGK8mZ8UcFK3+17uXhwx1nZ0IH06RjB0IlULb6hQlZ/MhYITKLNutMEhTP31NWJPhWpx VwXZwv4DXnkVmqO/q+uSCAZfuQ3FiocILzXbHPPcSFUbjgcRRAwRxjD0IAndAkibwb0c OKbNu7JJsmlJejwEHbn4fNwDbvayjBMnBYM2laFmrRtkpPc8Y6Mkh3QSzw75JzCzjdyY si9w== X-Gm-Message-State: AOJu0Ywre9LfWRRxnrLjilTxJGFZ9hKC8SXjDQYOePa1+xWX5e8LV7bG 4eL9iuZE0KMAiIqOuUTHf6ErMXw2q90Qlfx27hSD4vFgXNClasknv+tZy21wTu7E4S2IBBni7sW ua+s/ X-Gm-Gg: AY/fxX4zPkSgEt70csx1EPhOiAyg1zahkHoB9g5lCxAPLwu4OyufVmybVs/qEt6ESLQ JJ/7Nv537V2qntBdrgTejtna/28xjdpknzgVIZL7IihxlFJArc262k+dv8u8MRPpSwx/ioOkkWU tbssRM5Xh8oPM6RShWzLOETwQ7FOwyExf2mjTvGNu1tbJ6XIAqfDjlmphJmZFyVuuCLAPaqwIQj kk342gmgBYY98PVMzo1Gcco9vvyA5A5S+9Ki1PAlqLefFJn4llZn91bd9nAqRYAN62lpKAdYsrX 0lz8Zn71Nd+XbIUY05bdXQJXHPN+Mex6qCXtNmpta15lhh1hs/XYZW7J0XamuZwcTNmm/qAMlV2 OhnC+/SZbwH06+41ZBQ+M4EpScpARCCOhhG9i/gxDJpGmiSsrH1BBYTyinXHHvQyt6s2ZO0TBy/ GdAYpYu6gPtR76Bm+9F08iGHZJ3xUgdlmmAp0F5Dcve30FMQSjdWY+ X-Received: by 2002:a17:906:308c:b0:b87:6bea:d48c with SMTP id a640c23a62f3a-b876beade99mr155358566b.23.1768410844175; Wed, 14 Jan 2026 09:14:04 -0800 (PST) Received: from phoenix.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-b876838f431sm215169366b.9.2026.01.14.09.14.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 14 Jan 2026 09:14:03 -0800 (PST) Date: Wed, 14 Jan 2026 09:13:58 -0800 From: Stephen Hemminger To: Dimon Zhao Cc: dev@dpdk.org, Kyo Liu , Leon Yu , Sam Chen Subject: Re: [PATCH v2 1/1] net/nbl: fix issues reported by Coverity Message-ID: <20260114091358.14f4e118@phoenix.local> In-Reply-To: <20260114064425.45681-2-dimon.zhao@nebula-matrix.com> References: <20251030033619.3386064-1-dimon.zhao@nebula-matrix.com> <20260114064425.45681-1-dimon.zhao@nebula-matrix.com> <20260114064425.45681-2-dimon.zhao@nebula-matrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, 13 Jan 2026 22:44:25 -0800 Dimon Zhao wrote: > Coverity issue: 490942 > Coverity issue: 490943 > Coverity issue: 490946 > Coverity issue: 490947 > Coverity issue: 490949 > Coverity issue: 490950 > Coverity issue: 490951 > Coverity issue: 490952 > Coverity issue: 490953 > Coverity issue: 490954 > Coverity issue: 490955 > Coverity issue: 490957 > Coverity issue: 490958 > Coverity issue: 490959 > Fixes: a1c5ffa13b2c ("net/nbl: add channel layer") > Fixes: dc955cd24c8f ("net/nbl: add coexistence mode") > Fixes: 93b38df5a2ec ("net/nbl: support basic configuration") >=20 > Signed-off-by: Dimon Zhao > --- Thanks, AI review had some good feedback. Would you like me to add a summary to the commit, should have a description more than just Coverity ids. --- net/nbl: fix issues reported by Coverity Address multiple issues reported by Coverity static analysis: Check return values of ioctl() calls for NBL_DEV_USER_GET_BAR_SIZE and NBL_DEV_USER_CLEAR_EVENTFD. The original code tested the return value without capturing it from the ioctl call. Move debug log statement after NULL pointer validation in nbl_dev_configure() to prevent dereference of adapter before the check. Cast u16 operands to u64 before multiplication when calculating buffer sizes and offsets to prevent potential integer overflow. The num_txq_entries, num_rxq_entries, txq_buf_size, and rxq_buf_size fields are all u16 types, and their product could overflow before assignment to size_t or u64 destination variables. Coverity issue: 490942 Coverity issue: 490943 Coverity issue: 490946 Coverity issue: 490947 Coverity issue: 490949 Coverity issue: 490950 Coverity issue: 490951 Coverity issue: 490952 Coverity issue: 490953 Coverity issue: 490954 Coverity issue: 490955 Coverity issue: 490957 Coverity issue: 490958 Coverity issue: 490959 Fixes: a1c5ffa13b2c ("net/nbl: add channel layer") Fixes: dc955cd24c8f ("net/nbl: add coexistence mode") Fixes: 93b38df5a2ec ("net/nbl: support basic configuration") Cc: stable@dpdk.org Signed-off-by: Dimon Zhao --- # DPDK Patch Review: net/nbl: fix issues reported by Coverity ## Summary This patch addresses multiple Coverity static analysis issues in the NBL dr= iver, fixing unchecked return values and potential integer overflow conditi= ons. --- ## Commit Message Review ### Subject Line **`net/nbl: fix issues reported by Coverity`** (42 chars) | Check | Status | Notes | |-------|--------|-------| | Length =E2=89=A460 chars | =E2=9C=85 Pass | 42 characters | | Lowercase after colon | =E2=9C=85 Pass | | | Correct prefix (`net/nbl:`) | =E2=9C=85 Pass | | | Imperative mood | =E2=9C=85 Pass | | | No trailing period | =E2=9C=85 Pass | | | No forbidden punctuation | =E2=9C=85 Pass | | ### Commit Body | Check | Status | Notes | |-------|--------|-------| | `Coverity issue:` tags | =E2=9C=85 Pass | 14 Coverity issue tags present | | `Fixes:` tags | =E2=9C=85 Pass | 3 Fixes tags with 12-char SHA | | `Signed-off-by:` | =E2=9C=85 Pass | Present with real name and email | | Tag order | =E2=9C=85 Pass | Coverity =E2=86=92 Fixes =E2=86=92 Signed-of= f-by | | Body lines =E2=89=A475 chars | =E2=9C=85 Pass | | **Warning**: The commit body lacks a description explaining *what* the issu= es were (e.g., "unchecked return values," "potential integer overflow"). DP= DK convention strongly recommends describing the problem being fixed. Consi= der adding 1-2 sentences explaining: - The ioctl return values were not being checked - Integer multiplications could overflow before assignment to `size_t`/`u64` - Null pointer dereference before validation --- ## Code Review ### 1. nbl_userdev.c: Unchecked ioctl() Return Values **Change 1** (lines 82-103): `NBL_DEV_USER_GET_BAR_SIZE` ```c - ioctl(common->devfd, NBL_DEV_USER_GET_BAR_SIZE, &bar_size); - if (!ret) { + ret =3D ioctl(common->devfd, NBL_DEV_USER_GET_BAR_SIZE, &bar_size); + if (ret) { + NBL_LOG(ERR, "nbl userdev get bar size failed"); + goto close_eventfd; + } ``` | Assessment | Status | |------------|--------| | Correctness | =E2=9C=85 Good fix - the original code tested `ret` but nev= er captured the ioctl return | | Error handling | =E2=9C=85 Proper error path with goto cleanup | | Logic inversion | =E2=9C=85 Correct - changed from `if (!ret)` success pa= th to `if (ret)` error path | **Change 2** (lines 118-120): `NBL_DEV_USER_CLEAR_EVENTFD` ```c - ioctl(common->devfd, NBL_DEV_USER_CLEAR_EVENTFD, 0); + ret =3D ioctl(common->devfd, NBL_DEV_USER_CLEAR_EVENTFD, 0); + if (ret) + NBL_LOG(ERR, "nbl userdev set clear eventfd failed, ret: %d", ret); ``` | Assessment | Status | |------------|--------| | Correctness | =E2=9C=85 Good - captures return value and logs error | | Error handling | =E2=9A=A0=EF=B8=8F Minor - logs but continues cleanup; a= cceptable in teardown path | --- ### 2. nbl_dev.c: Null Pointer Dereference Before Check **Change** (lines 132-138): ```c - NBL_LOG(DEBUG, "Begin to configure the device, state: %d", adapter->state= ); - if (dev_data =3D=3D NULL || adapter =3D=3D NULL) return -EINVAL; + NBL_LOG(DEBUG, "Begin to configure the device, state: %d", adapter->state= ); ``` | Assessment | Status | |------------|--------| | Correctness | =E2=9C=85 Good fix - `adapter->state` was dereferenced befo= re NULL check | | DPDK style | =E2=9A=A0=EF=B8=8F Warning - Uses `=3D=3D NULL` (correct per= AGENTS.md) | --- ### 3. nbl_channel.c: Integer Overflow Fixes These fix potential integer overflow when multiplying `u16` values before w= idening. **Change 1** (line 151): `nbl_chan_init_tx_queue` ```c - size =3D chan_info->mailbox.num_txq_entries * chan_info->mailbox.txq_buf_= size; + size =3D (u64)chan_info->mailbox.num_txq_entries * (u64)chan_info->mailbo= x.txq_buf_size; ``` **Change 2** (line 160): `nbl_chan_init_rx_queue` ```c - size =3D chan_info->mailbox.num_rxq_entries * chan_info->mailbox.rxq_buf_= size; + size =3D (u64)chan_info->mailbox.num_rxq_entries * (u64)chan_info->mailbo= x.rxq_buf_size; ``` **Change 3** (line 169): `nbl_chan_prepare_rx_bufs` loop ```c - desc[i].buf_addr =3D rxq->buf_mem.pa + i * chan_info->mailbox.rxq_buf_siz= e; + desc[i].buf_addr =3D rxq->buf_mem.pa + (u64)i * (u64)chan_info->mailbox.r= xq_buf_size; ``` **Change 4** (lines 178-179): `nbl_chan_advance_rx_ring` ```c - rx_desc->buf_addr =3D rxq->buf_mem.pa + chan_info->mailbox.rxq_buf_size *= next_to_use; + rx_desc->buf_addr =3D rxq->buf_mem.pa + + (u64)chan_info->mailbox.rxq_buf_size * (u64)next_to_use; ``` **Change 5** (lines 189-190): `nbl_chan_update_txqueue` ```c - va =3D (u8 *)txq->buf + next_to_use * chan_info->mailbox.txq_buf_size; - pa =3D txq->buf_mem.pa + next_to_use * chan_info->mailbox.txq_buf_size; + va =3D (u8 *)txq->buf + (u64)next_to_use * (u64)chan_info->mailbox.txq_bu= f_size; + pa =3D txq->buf_mem.pa + (u64)next_to_use * (u64)chan_info->mailbox.txq_b= uf_size; ``` | Assessment | Status | |------------|--------| | Correctness | =E2=9C=85 Fixes overflow: `u16 * u16` can exceed 32 bits wi= th large values | | Consistency | =E2=9A=A0=EF=B8=8F Warning - **Incomplete fix**: Similar pa= tterns exist in the same file that were NOT patched | ### **Missing Fixes** (same overflow pattern still present): 1. **Line 350** in `nbl_chan_clean_queue`: ```c data =3D (u8 *)rxq->buf + next_to_clean * chan_info->mailbox.rxq_buf_siz= e; ``` =20 2. **Line 361** in same function: ```c data =3D (u8 *)rxq->buf + next_to_clean * chan_info->mailbox.rxq_buf_siz= e; ``` These have the same `u16 * u16` multiplication pattern and should also be f= ixed for consistency. --- ## Verdict ### Errors (Must Fix) None - patch is acceptable. ### Warnings (Should Fix) 1. **Incomplete fix**: Lines 350 and 361 in `nbl_channel.c` have the same i= nteger overflow pattern but are not addressed. Recommend adding them to thi= s patch for completeness. 2. **Missing commit body description**: The commit message lists Coverity I= Ds but doesn't describe what classes of issues are being fixed. Add a sente= nce like: > "Fix unchecked ioctl() return values, null pointer dereference before = validation, and potential integer overflow in buffer size calculations." 3. **Consider `Cc: stable@dpdk.org`**: Since these are bug fixes with `Fixe= s:` tags, this patch may be a candidate for stable backport. ### Info (Consider) - The dual cast `(u64)a * (u64)b` pattern is verbose but correct. An altern= ative is casting just the first operand: `(u64)a * b` (the result promotes = to u64). Both are acceptable. --- ## Recommendation **Acked-with-comments**: The patch is technically correct and addresses the= reported Coverity issues. Recommend: 1. Add the two missing overflow fixes at lines 350/361 for completeness 2. Add a brief description to the commit body 3. Consider `Cc: stable@dpdk.org`