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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 12740C19F28 for ; Wed, 3 Aug 2022 19:39:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238466AbiHCTjN (ORCPT ); Wed, 3 Aug 2022 15:39:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41160 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238444AbiHCTjL (ORCPT ); Wed, 3 Aug 2022 15:39:11 -0400 Received: from mail-io1-xd2e.google.com (mail-io1-xd2e.google.com [IPv6:2607:f8b0:4864:20::d2e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CDE69165AF for ; Wed, 3 Aug 2022 12:39:09 -0700 (PDT) Received: by mail-io1-xd2e.google.com with SMTP id h139so179244iof.12 for ; Wed, 03 Aug 2022 12:39:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel-dk.20210112.gappssmtp.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=fSy7JOI2OPrBvYYSmGHOL+R7BpjU4u61ICdJviI28dU=; b=UVi/S2oOqwI47l5CWS7rAmwDkC+xVfeWVNsUO2aK6oiFW7r9dbtjz0QDsbNz/S3Wwu VMniJDi3d8L6RPlEhMm52w4lgLo4ArrVgxqK1Kpn2ulA3PLzfjBg08arK0NISmUDDymB /WtkZBnE1WEFto+Zgk2UuMFf4CX80IrB3XG7PNE0dm6OTPU8oWFT5z2cwC5oWFSt4kVY RpzNCWBypdVhu/nPL6/k+lFW7Liuyi6R39nkpgp1hd/ttgiHsWmkLKOrv2XNjWaXhngX 63P96uw6WlRbRF/czLxVhQDHGul8/QjlKeJZbwPTCHdB3Jkv7ttcQ7UBK+YM8Pyhakue bnRA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=fSy7JOI2OPrBvYYSmGHOL+R7BpjU4u61ICdJviI28dU=; b=wyPTuF19DZgo7ME+6j99OiYes9EAlHIKFk5pPrPp3+6lDhCmQZUHxONK9f9mD1/4F3 NcblJOaGNrZuRELPz/DzNtEfX/RXcojYqQg7chdtYR65A4QMeOX1j5yJVQ1U1CCAobdp PuAUeNsBw1cJSHXsrXrJDfBVYknZSpUshSZn3mjBt7XjKyhLe7F0Ml++k1DgsCejj84O ux7INIFsU7xP+lwa5vGohWQVqoy7bcd4METAwZNTUu+knJI11nyoJwHpsM2f91o6kx0M 0fs2hdNMFJ0vxsNsp6XY1fjG4WJF1o6rF8tuzu32VYbee6V2DioTw2zZTE2xqepRs8Y4 Qy1w== X-Gm-Message-State: ACgBeo0XkbWgw21O9a+YSi1F7qAJYOhAEnJmhhG96ufk3nDf2K3ly9/D qj7ZkhlQ+r5+LVaWsY4AUKF+SQ== X-Google-Smtp-Source: AA6agR5yz6fayuEmHNxdmTfro1gBVCR4ygOjwcExgpR5dMILeYMqFiapzUwtu7CJooNgvW9kytirsg== X-Received: by 2002:a05:6638:5a4:b0:342:7040:9a04 with SMTP id b4-20020a05663805a400b0034270409a04mr7965064jar.196.1659555549201; Wed, 03 Aug 2022 12:39:09 -0700 (PDT) Received: from [192.168.1.172] ([207.135.234.126]) by smtp.gmail.com with ESMTPSA id s13-20020a02b14d000000b0034277c336b0sm3856738jah.58.2022.08.03.12.39.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 03 Aug 2022 12:39:08 -0700 (PDT) Message-ID: <54d7ec45-3f69-294b-7036-b9350cb1ab4c@kernel.dk> Date: Wed, 3 Aug 2022 13:39:08 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH] audit, io_uring, io-wq: Fix memory leak in io_sq_thread() and io_wqe_worker() Content-Language: en-US To: Paul Moore , Peilin Ye Cc: Pavel Begunkov , Eric Paris , Peilin Ye , io-uring@vger.kernel.org, linux-kernel@vger.kernel.org, linux-audit@redhat.com References: <20220803050230.30152-1-yepeilin.cs@gmail.com> From: Jens Axboe In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: io-uring@vger.kernel.org On 8/3/22 1:28 PM, Paul Moore wrote: > On Wed, Aug 3, 2022 at 9:16 AM Paul Moore wrote: >> On Wed, Aug 3, 2022 at 1:03 AM Peilin Ye wrote: >>> >>> Currently @audit_context is allocated twice for io_uring workers: >>> >>> 1. copy_process() calls audit_alloc(); >>> 2. io_sq_thread() or io_wqe_worker() calls audit_alloc_kernel() (which >>> is effectively audit_alloc()) and overwrites @audit_context, >>> causing: >>> >>> BUG: memory leak >>> unreferenced object 0xffff888144547400 (size 1024): >>> <...> >>> hex dump (first 32 bytes): >>> 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 ................ >>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>> backtrace: >>> [] audit_alloc+0x133/0x210 >>> [] copy_process+0xcd3/0x2340 >>> [] create_io_thread+0x63/0x90 >>> [] create_io_worker+0xb4/0x230 >>> [] io_wqe_enqueue+0x248/0x3b0 >>> [] io_queue_iowq+0xba/0x200 >>> [] io_queue_async+0x113/0x180 >>> [] io_req_task_submit+0x18f/0x1a0 >>> [] io_apoll_task_func+0xdd/0x120 >>> [] tctx_task_work+0x11f/0x570 >>> [] task_work_run+0x7e/0xc0 >>> [] get_signal+0xc18/0xf10 >>> [] arch_do_signal_or_restart+0x2b/0x730 >>> [] exit_to_user_mode_prepare+0x5e/0x180 >>> [] syscall_exit_to_user_mode+0x12/0x20 >>> [] do_syscall_64+0x40/0x80 >>> >>> Then, >>> >>> 3. io_sq_thread() or io_wqe_worker() frees @audit_context using >>> audit_free(); >>> 4. do_exit() eventually calls audit_free() again, which is okay >>> because audit_free() does a NULL check. >>> >>> Free the old @audit_context first in audit_alloc_kernel(), and delete >>> the redundant calls to audit_free() for less confusion. >>> >>> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Peilin Ye >>> --- >>> Hi all, >>> >>> A better way to fix this memleak would probably be checking >>> @args->io_thread in copy_process()? Something like: >>> >>> if (args->io_thread) >>> retval = audit_alloc_kernel(); >>> else >>> retval = audit_alloc(); >>> >>> But I didn't want to add another if to copy_process() for this bugfix. >>> Please suggest, thanks! >> >> Thanks for the report and patch! I'll take a closer look at this >> today and get back to you. > > I think the best solution to this is simply to remove the calls to > audit_alloc_kernel() in the io_uring and io-wq code, as well as the > audit_alloc_kernel() function itself. As long as create_io_thread() > ends up calling copy_process to create the new kernel thread the > audit_context should be allocated correctly. Peilin Ye, are you able > to draft a patch to do that and give it a test? > > For those that may be wondering how this happened (I definitely was!), > it looks like when I first started working on the LSM/audit support > for io_uring it was before the v5.12-rc1 release when > create_io_thread() was introduced. Prior to create_io_thread() it > appears that io_uring/io-wq wasn't calling into copy_process() and > thus was not getting an audit_context allocated in the kernel thread's > task_struct; the solution for those original development drafts was to > add a call to a new audit_alloc_kernel() which would handle the > audit_context allocation. Unfortunately, I didn't notice the move to > create_io_thread() during development and the redundant > audit_alloc_kernel() calls remained :/ I agree with your analysis and suggested solution. Post the native io-wq workers create_io_thread() -> copy_process() is always used for io-wq (and sqpoll, for that matter). -- Jens Axboe 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 us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D5C7EC19F28 for ; Wed, 3 Aug 2022 20:09:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1659557388; h=from:from:sender:sender: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:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=9sQwFMCk6kq0mqY1bUWpOQhaNazaPFLMWrecOwckfzg=; b=KraHWJ0oPkT1tN3FLRuKdEI1XhD2POdgBGwViDzl0iHMtO3hUFPDA1e63fLhCIDZakAHvA SiYFt3zQjELMw1HiuP3deKt76I0lXSA0zudA9txA8wlyLFaZR2bJSigQ1XO5OAWXRY0RWj u0/KR04k+22BJQEfFeLjJk31oUnEWOQ= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-386-F7j7N9fwM3eFHwHhT9SLrg-1; Wed, 03 Aug 2022 16:09:45 -0400 X-MC-Unique: F7j7N9fwM3eFHwHhT9SLrg-1 Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id ED73018A6582; Wed, 3 Aug 2022 20:09:43 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (unknown [10.30.29.100]) by smtp.corp.redhat.com (Postfix) with ESMTP id AE2F9492C3B; Wed, 3 Aug 2022 20:09:42 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (localhost [IPv6:::1]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id 6E0731946A52; Wed, 3 Aug 2022 20:09:42 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id 13EB21946A52 for ; Wed, 3 Aug 2022 19:39:12 +0000 (UTC) Received: by smtp.corp.redhat.com (Postfix) id 041CC1410F3C; Wed, 3 Aug 2022 19:39:12 +0000 (UTC) Received: from mimecast-mx02.redhat.com (mimecast02.extmail.prod.ext.rdu2.redhat.com [10.11.55.18]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 009141410F38 for ; Wed, 3 Aug 2022 19:39:11 +0000 (UTC) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id DE011801231 for ; Wed, 3 Aug 2022 19:39:11 +0000 (UTC) Received: from mail-io1-f53.google.com (mail-io1-f53.google.com [209.85.166.53]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-358-QWNW9_G-O8K8D2avbbEgTw-1; Wed, 03 Aug 2022 15:39:10 -0400 X-MC-Unique: QWNW9_G-O8K8D2avbbEgTw-1 Received: by mail-io1-f53.google.com with SMTP id p81so13674017iod.2 for ; Wed, 03 Aug 2022 12:39:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=fSy7JOI2OPrBvYYSmGHOL+R7BpjU4u61ICdJviI28dU=; b=sJm+P3YQbqAGkyywFbkxZAA2RSwa5NWa7r+1qYhqxHFSWvGHnWADTUgSWW8ZKdUmZJ Dj5mxoAhTfbD4ujLjJqrz7PP96nTQ5kRZ2X5Yd28v3lK93cPNhr9hhynF9Ai20/zaQGd Up1g5PeVPzIZq7pTTo3iY9HeM3srRFZy+JPLK2V+pK5RTGj45zmQgldhNR2kqPTFirZJ krzD9eWeI8JDUmgK0o/hutsnu6lLgchR8uAZExn+Vij8d5762YhZkk14fhin2v4b0y+I xBaKBiltPcSNSH0s0tngox6j9nchW5p0zFfS+dbBQVYtxeSfw62RjIOy6il+MWqtqbXv 6okw== X-Gm-Message-State: ACgBeo3/rnZ5co4kSNhP9MABw9y02aRqL9g8UawZbOJPXfF8LdE/nBkY igsBKs1Zeb1Ry3fTyme8zQIPecwvF6qrqA== X-Google-Smtp-Source: AA6agR5yz6fayuEmHNxdmTfro1gBVCR4ygOjwcExgpR5dMILeYMqFiapzUwtu7CJooNgvW9kytirsg== X-Received: by 2002:a05:6638:5a4:b0:342:7040:9a04 with SMTP id b4-20020a05663805a400b0034270409a04mr7965064jar.196.1659555549201; Wed, 03 Aug 2022 12:39:09 -0700 (PDT) Received: from [192.168.1.172] ([207.135.234.126]) by smtp.gmail.com with ESMTPSA id s13-20020a02b14d000000b0034277c336b0sm3856738jah.58.2022.08.03.12.39.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 03 Aug 2022 12:39:08 -0700 (PDT) Message-ID: <54d7ec45-3f69-294b-7036-b9350cb1ab4c@kernel.dk> Date: Wed, 3 Aug 2022 13:39:08 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH] audit, io_uring, io-wq: Fix memory leak in io_sq_thread() and io_wqe_worker() To: Paul Moore , Peilin Ye References: <20220803050230.30152-1-yepeilin.cs@gmail.com> From: Jens Axboe In-Reply-To: X-Mimecast-Impersonation-Protect: Policy=CLT - Impersonation Protection Definition; Similar Internal Domain=false; Similar Monitored External Domain=false; Custom External Domain=false; Mimecast External Domain=false; Newly Observed Domain=false; Internal User Name=false; Custom Display Name List=false; Reply-to Address Mismatch=false; Targeted Threat Dictionary=false; Mimecast Threat Dictionary=false; Custom Threat Dictionary=false X-Scanned-By: MIMEDefang 2.85 on 10.11.54.7 X-Mailman-Approved-At: Wed, 03 Aug 2022 20:09:41 +0000 X-BeenThere: linux-audit@redhat.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux Audit Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-kernel@vger.kernel.org, Eric Paris , io-uring@vger.kernel.org, linux-audit@redhat.com, Peilin Ye , Pavel Begunkov Errors-To: linux-audit-bounces@redhat.com Sender: "Linux-audit" X-Scanned-By: MIMEDefang 2.85 on 10.11.54.10 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On 8/3/22 1:28 PM, Paul Moore wrote: > On Wed, Aug 3, 2022 at 9:16 AM Paul Moore wrote: >> On Wed, Aug 3, 2022 at 1:03 AM Peilin Ye wrote: >>> >>> Currently @audit_context is allocated twice for io_uring workers: >>> >>> 1. copy_process() calls audit_alloc(); >>> 2. io_sq_thread() or io_wqe_worker() calls audit_alloc_kernel() (which >>> is effectively audit_alloc()) and overwrites @audit_context, >>> causing: >>> >>> BUG: memory leak >>> unreferenced object 0xffff888144547400 (size 1024): >>> <...> >>> hex dump (first 32 bytes): >>> 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 ................ >>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>> backtrace: >>> [] audit_alloc+0x133/0x210 >>> [] copy_process+0xcd3/0x2340 >>> [] create_io_thread+0x63/0x90 >>> [] create_io_worker+0xb4/0x230 >>> [] io_wqe_enqueue+0x248/0x3b0 >>> [] io_queue_iowq+0xba/0x200 >>> [] io_queue_async+0x113/0x180 >>> [] io_req_task_submit+0x18f/0x1a0 >>> [] io_apoll_task_func+0xdd/0x120 >>> [] tctx_task_work+0x11f/0x570 >>> [] task_work_run+0x7e/0xc0 >>> [] get_signal+0xc18/0xf10 >>> [] arch_do_signal_or_restart+0x2b/0x730 >>> [] exit_to_user_mode_prepare+0x5e/0x180 >>> [] syscall_exit_to_user_mode+0x12/0x20 >>> [] do_syscall_64+0x40/0x80 >>> >>> Then, >>> >>> 3. io_sq_thread() or io_wqe_worker() frees @audit_context using >>> audit_free(); >>> 4. do_exit() eventually calls audit_free() again, which is okay >>> because audit_free() does a NULL check. >>> >>> Free the old @audit_context first in audit_alloc_kernel(), and delete >>> the redundant calls to audit_free() for less confusion. >>> >>> Fixes: 5bd2182d58e9 ("audit,io_uring,io-wq: add some basic audit support to io_uring") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Peilin Ye >>> --- >>> Hi all, >>> >>> A better way to fix this memleak would probably be checking >>> @args->io_thread in copy_process()? Something like: >>> >>> if (args->io_thread) >>> retval = audit_alloc_kernel(); >>> else >>> retval = audit_alloc(); >>> >>> But I didn't want to add another if to copy_process() for this bugfix. >>> Please suggest, thanks! >> >> Thanks for the report and patch! I'll take a closer look at this >> today and get back to you. > > I think the best solution to this is simply to remove the calls to > audit_alloc_kernel() in the io_uring and io-wq code, as well as the > audit_alloc_kernel() function itself. As long as create_io_thread() > ends up calling copy_process to create the new kernel thread the > audit_context should be allocated correctly. Peilin Ye, are you able > to draft a patch to do that and give it a test? > > For those that may be wondering how this happened (I definitely was!), > it looks like when I first started working on the LSM/audit support > for io_uring it was before the v5.12-rc1 release when > create_io_thread() was introduced. Prior to create_io_thread() it > appears that io_uring/io-wq wasn't calling into copy_process() and > thus was not getting an audit_context allocated in the kernel thread's > task_struct; the solution for those original development drafts was to > add a call to a new audit_alloc_kernel() which would handle the > audit_context allocation. Unfortunately, I didn't notice the move to > create_io_thread() during development and the redundant > audit_alloc_kernel() calls remained :/ I agree with your analysis and suggested solution. Post the native io-wq workers create_io_thread() -> copy_process() is always used for io-wq (and sqpoll, for that matter). -- Jens Axboe -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit