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 6D825A0566; Tue, 28 Jun 2022 13:22:48 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1023840691; Tue, 28 Jun 2022 13:22:48 +0200 (CEST) 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 77CC0400D7 for ; Tue, 28 Jun 2022 13:22:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1656415366; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5eJ5tpcBarijFD3gU6+cWy2cKAjyB8g1IVDLV3NVJck=; b=LJ1ZPs/iHS28izrje0Xc9N+VNv7cztPrnd1VyGWfS/liROpvxlZGVfcp9F4sJWRhHexbqI OlOqSdOgahsHVD1EDJD9xZ/hzJqqeq1h8JdyCOOyMBhNRmZ4tGY5uwUdSJwSOPyobC14Q7 H/D4ZDwxwx6xbyJAIkze7LoDtPtHnrM= Received: from mail-lj1-f199.google.com (mail-lj1-f199.google.com [209.85.208.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-672-GwH6LxQwMZKjqQ7v1vqd5w-1; Tue, 28 Jun 2022 07:22:42 -0400 X-MC-Unique: GwH6LxQwMZKjqQ7v1vqd5w-1 Received: by mail-lj1-f199.google.com with SMTP id k12-20020a05651c10ac00b0025a73553415so1552111ljn.5 for ; Tue, 28 Jun 2022 04:22:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=5eJ5tpcBarijFD3gU6+cWy2cKAjyB8g1IVDLV3NVJck=; b=IrNeGWSzgGB58dHHo4J8vm589ghkLwYhLkLJ3nKu3yekqncMOHX1OA16JoGEnsR4m/ U2GE34Vo+q3g4IRFb4yIvBTpxNj6AgQOiTbURE5I3fEFBs9xVwnbmN2gDvMZdsNyOjO2 WgBU9C2IiFX2UVT+EbgKwBeXLEsTklAuHBctab/Lgfz7tJVVzcmVB+wwQQQzMsqvbXzd FlZR6SjYjfiFN0njUvPr+IDdbcaIi+f4kyfNRzKPcDVYoxKSPK9lC6YQX7GiJ5QZDPIa i/0jEkBXCzKIOU+ErgsHiwUKXNsS+8RXCGcGw/ND2ZIyZq4j/8iLw2ihvk9yeHd1Ubsr w54Q== X-Gm-Message-State: AJIora9p3nz2xs68CdIAhjB30t1wKnhwxYfOJqrwOloUCc0aKmd+GVyh nqjmTNq8aUmysfmpob/YAt7Dx2QoWDJcwcYMazoPYICjmvqNhITcttrwMQlI3cqpogjJ5p+Hvrx Tf4c0gAVfePomTlkl9sU= X-Received: by 2002:ac2:4e97:0:b0:47f:b53b:4af5 with SMTP id o23-20020ac24e97000000b0047fb53b4af5mr11291993lfr.499.1656415361319; Tue, 28 Jun 2022 04:22:41 -0700 (PDT) X-Google-Smtp-Source: AGRyM1ulyLrqidEi/oCvT6X6LAw62YepZ84lj+UmxBEmbTTv03fghmX1DbgIAJhog7yLo9nEyIhEFMwZnP0Kjayeupo= X-Received: by 2002:ac2:4e97:0:b0:47f:b53b:4af5 with SMTP id o23-20020ac24e97000000b0047fb53b4af5mr11291972lfr.499.1656415361076; Tue, 28 Jun 2022 04:22:41 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: David Marchand Date: Tue, 28 Jun 2022 13:22:29 +0200 Message-ID: Subject: Re: [PATCH] crypto/qat: fix docsis segmentation fault To: "Troy, Rebecca" Cc: "Zhang, Roy Fan" , "Kusztal, ArkadiuszX" , "Trahe, Fiona" , Tomasz Jozwiak , "dev@dpdk.org" Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com 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, Jun 28, 2022 at 12:32 PM Troy, Rebecca wro= te: > > > On Mon, Jun 27, 2022 at 6:45 PM Rebecca Troy > > wrote: > > > > > > Currently if AES or DES algorithms fail for Docsis test suite, a > > > segmentation fault occurs when cryptodev_qat_autotest is ran. > > > > > > This is due to a duplicate call of EVP_CIPHER_CTX_free for the sessio= n > > > context. Ctx is freed firstly in the bpi_cipher_ctx_init function and > > > then again at the end of qat_sym_session_configure_cipher function. > > > > > > This commit fixes this bug by removing the first instance of > > > EVP_CIPHER_CTX_free, leaving just the dedicated function in the upper > > > level to free the ctx. > > > > This is awkward. > > This helper should let *ctx alone until everything succeeded. > > > > -- > > David Marchand > > Hi David, > > This bug was found under unusual circumstances. Unit tests failed due to = no legacy algorithm support on the system, which caused a segmentation faul= t rather than the expected 'Tests failed' result. > > When these unit tests fail, the current error handling directs that the *= ctx be freed twice - once prematurely (which is the instance this patch is = removing) and then again after the tests have registered as failed, causing= the segmentation fault. I agree that the *ctx shouldn't have been freed pr= ematurely, so this patch fixes the incorrect error handling here by removin= g that first instance of *ctx freeing. My point is that bpi_cipher_ctx_init should not return an allocated object on failure and hope that the caller would free it. This is counter intuitive. If someone starts using this helper somewhere else in the code and does not free the ctx on failure, we will have a leak. I see two alternatives to fix the issue in a cleaner way: - either set *ctx to NULL on free, diff --git a/drivers/crypto/qat/qat_sym_session.c b/drivers/crypto/qat/qat_sym_session.c index e40c042ba9..b30396487e 100644 --- a/drivers/crypto/qat/qat_sym_session.c +++ b/drivers/crypto/qat/qat_sym_session.c @@ -136,8 +136,10 @@ bpi_cipher_ctx_init(enum rte_crypto_cipher_algorithm cryptodev_algo, return 0; ctx_init_err: - if (*ctx !=3D NULL) + if (*ctx !=3D NULL) { EVP_CIPHER_CTX_free(*ctx); + *ctx =3D NULL; + } return ret; } - or use a temp variable for the allocated cipher ctx object and store it to *ctx only before returning 0, which was what I meant with my initial comment. --=20 David Marchand