From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0A30DA04FD; Thu, 10 Nov 2022 10:48:53 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F40D540143; Thu, 10 Nov 2022 10:48:52 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 5D1B2400D4 for ; Thu, 10 Nov 2022 10:48:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668073730; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=b5aJ0UmSqAETl/8Ur3/aU8NYaHRwuzF4HSEbQWjgnlw=; b=gMBjZ3dABan0Rsqal6zTuYgV9tY89APlz5tG1tQzzr+jrnOhUwkhNyNLSGyIwJcYUaYqiB vq5G9jNDDcrro3zs9jmDlZR5yCYNlykUrs9BsiXk7PNUQbJf0jgtVODbjd4vT7YFoeRoJp t6aG9Gydebu034OPuUmsAIdMjTNHbWM= Received: from mail-pl1-f199.google.com (mail-pl1-f199.google.com [209.85.214.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-434-1xqmF2u0P7esrqgH2_-JDQ-1; Thu, 10 Nov 2022 04:48:49 -0500 X-MC-Unique: 1xqmF2u0P7esrqgH2_-JDQ-1 Received: by mail-pl1-f199.google.com with SMTP id d18-20020a170902ced200b001871dab2d59so1058111plg.22 for ; Thu, 10 Nov 2022 01:48:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=b5aJ0UmSqAETl/8Ur3/aU8NYaHRwuzF4HSEbQWjgnlw=; b=x/NCxv1XUjPIW0uXKwN5mmIPXMzuyzC83UB+BVQUohjQy4kHlyUJRDlAYsSjaOYOmN JdCdLoVs5FNPnuPwtYkVCuH4SIbvuZfJvsqVpJV+bSdy4DgbkExgFqGPaVrfhG/TzUQr PEC101ygRKzmV9uBIS+vkftFGK4sOQiGqZpF+s+3l2aTQ7j/k0bacHkVfd6ka6Nh3dTk 8n6tVPDmfCV/frA1pvLapRSG5XQQ7ypowY/eU/s+Uf9RvGdZDzkKet+/43n3VBoXLTTC YXFt8mZr1TKzvWY/IWdGM4DTJTNcYlvL3FWSD8m96qnhyN8KUPFt8a0VgBeE5vgOdrv3 CTGA== X-Gm-Message-State: ANoB5plO+9pFh+IsSh2sn2rqlj0pYW38MAQddp83e94cqjHF9A7cHJDP OfcMv9nDp+2xRmtYeKkBJBcyQt4ERWtI3YjMucslCiI/Zxa31QhaU2O1enJIRasVamlvQ7nyUr0 OQuZUcuAM91kztxIzMp8= X-Received: by 2002:a63:1b15:0:b0:470:75a1:c6d7 with SMTP id b21-20020a631b15000000b0047075a1c6d7mr1536625pgb.120.1668073728603; Thu, 10 Nov 2022 01:48:48 -0800 (PST) X-Google-Smtp-Source: AA0mqf4doOGVkNfFMhChp9UhnEAvh8Qt9mkga/jdYFhbHss/UhlehcF6pjQNtI8k0FqcuiaauDFGNCKoWTFNQKg3ELI= X-Received: by 2002:a63:1b15:0:b0:470:75a1:c6d7 with SMTP id b21-20020a631b15000000b0047075a1c6d7mr1536608pgb.120.1668073728239; Thu, 10 Nov 2022 01:48:48 -0800 (PST) MIME-Version: 1.0 References: <20221104035209.62109-1-hernan.vargas@intel.com> <20221104035209.62109-2-hernan.vargas@intel.com> In-Reply-To: <20221104035209.62109-2-hernan.vargas@intel.com> From: David Marchand Date: Thu, 10 Nov 2022 10:48:36 +0100 Message-ID: Subject: Re: [PATCH v1 1/1] baseband/acc: fix check after deref and dead code To: Hernan Vargas Cc: dev@dpdk.org, gakhil@marvell.com, trix@redhat.com, maxime.coquelin@redhat.com, nicolas.chautru@intel.com, qi.z.zhang@intel.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" 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 Thu, Nov 3, 2022 at 8:57 PM Hernan Vargas wrote: > > Fix potential issue of dereferencing a pointer before null check. > Remove null check for value that could never be null. > > Coverity issue: 381646, 381631 > Fixes: 989dec301a9 ("baseband/acc100: add ring companion address") > > Signed-off-by: Hernan Vargas > --- > drivers/baseband/acc/rte_acc100_pmd.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/baseband/acc/rte_acc100_pmd.c b/drivers/baseband/acc/rte_acc100_pmd.c > index 96daef87bc..30a718916d 100644 > --- a/drivers/baseband/acc/rte_acc100_pmd.c > +++ b/drivers/baseband/acc/rte_acc100_pmd.c > @@ -4122,15 +4122,11 @@ acc100_dequeue_ldpc_enc(struct rte_bbdev_queue_data *q_data, > struct rte_bbdev_enc_op *op; > union acc_dma_desc *desc; > > - if (q == NULL) > - return 0; I guess this protects badly written applications that would do stuff like pass an incorrect queue id, or call this callback while the queue has not been configured yet. This is something that should be caught at the bbdev layer (arguably under the RTE_LIBRTE_BBDEV_DEBUG if the performance is that much affected, though I'd like to see numbers). (edit: I see Maxime replied a similar comment). Back to this particular patch, rather than remove the check, the right fix is to move acc_ring_avail_deq(q). This is what Coverity reports. And this same pattern is used in other parts of the driver. It just happens that Coverity did not report them because some avec under RTE_LIBRTE_BBDEV_DEBUG... > #ifdef RTE_LIBRTE_BBDEV_DEBUG > if (unlikely(ops == 0)) And I also noticed this hunk. DPDK coding style, ops should be compared against NULL, but see below... > return 0; > #endif > desc = q->ring_addr + (q->sw_ring_tail & q->sw_ring_wrap_mask); > - if (unlikely(desc == NULL)) > - return 0; > op = desc->req.op_addr; > if (unlikely(ops == NULL || op == NULL)) > return 0; ... like here, so above check is redundant. There is probably more cleanups to do in this driver. This can be done later. -- David Marchand