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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id C01B8C433F5 for ; Thu, 17 Mar 2022 05:47:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DC23E6B0071; Thu, 17 Mar 2022 01:47:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D71EF6B0072; Thu, 17 Mar 2022 01:47:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C13368D0001; Thu, 17 Mar 2022 01:47:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0046.hostedemail.com [216.40.44.46]) by kanga.kvack.org (Postfix) with ESMTP id AD7116B0071 for ; Thu, 17 Mar 2022 01:47:45 -0400 (EDT) Received: from smtpin28.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 632FAA3086 for ; Thu, 17 Mar 2022 05:47:45 +0000 (UTC) X-FDA: 79252796448.28.679CAED Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf31.hostedemail.com (Postfix) with ESMTP id C7D1C20016 for ; Thu, 17 Mar 2022 05:47:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1647496064; 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=j/KLDc89By4qA4kGRzPodGb4mNYSnQmxLVR6xOnnWng=; b=XVwx10HLw6dv7pR45dpAg6e6/rPCcjzX3rNA0SoPD/fqJedVbi0XAUyzD+OQfMocBsu4N6 10FGt5JJ1yjZelHtqngXFzlz2Mc12j6vj493/rgVSJ7eYLPKPrCLMaqLiuFRimZAWGUei0 S8EB/m13qqq/XTpsDVPycg4bbs0/a04= Received: from mail-lj1-f197.google.com (mail-lj1-f197.google.com [209.85.208.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-573-fXlnULrSOhyxuPFLYZBjJw-1; Thu, 17 Mar 2022 01:47:41 -0400 X-MC-Unique: fXlnULrSOhyxuPFLYZBjJw-1 Received: by mail-lj1-f197.google.com with SMTP id 6-20020a2eb946000000b002463d2915d2so1717854ljs.9 for ; Wed, 16 Mar 2022 22:47:41 -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=j/KLDc89By4qA4kGRzPodGb4mNYSnQmxLVR6xOnnWng=; b=fVitHNxa9+7oifwD8E7xGpSCjjoN+eSV6b0m2aSalR5zWC3/r1ZLgw2vRfYFal9cOY H4DTvqM23MBCjhk8ERkbDH3I2OymGRufd/YU5OTlN57W5MxiFtDBD4ETjOw4ZHuwGSfd +lah0I5+qWmNHAAPcj/PxM6a5xyrqB/OCUZ1lWnXfsAfTKAKxQJZO5WKNvtw87pFQ0Vy /tTOOt6gsGbTadYG7pqbXHjBI96hV0R5pVLCBj3Ca/MKoGGsiWHjxeipEvLnuFFdw3MB z8QXBMs/J4l8TEc5kCQ3ZZHk9ApuPgRLxQSvkLjf3viRHzsvShvPkjWDLy7GtULnVenn CKeA== X-Gm-Message-State: AOAM5335IjCX6/LWYFIVynNeSX/jU6XRQhkdqVkeBffuGdlTxbmy0mEr 5IMbQPb7MgqorM+Z7eCDrQL/TWTA2ve9gw2e0DV4kgfAvpS5UkihZlc8bI9b8+MMImorcELXe1o LPOxoPi4okQ== X-Received: by 2002:a05:6512:3981:b0:448:40e5:cf90 with SMTP id j1-20020a056512398100b0044840e5cf90mr1787576lfu.656.1647496060061; Wed, 16 Mar 2022 22:47:40 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwdTK5wvVLygRRXRR1UQYzsP+oY3rMafVhQyvquWomXKKQlBZlVJXihNGbpkr34wSeZNlbJzA== X-Received: by 2002:a05:6512:3981:b0:448:40e5:cf90 with SMTP id j1-20020a056512398100b0044840e5cf90mr1787558lfu.656.1647496059721; Wed, 16 Mar 2022 22:47:39 -0700 (PDT) Received: from [192.168.1.121] (91-145-109-188.bb.dnainternet.fi. [91.145.109.188]) by smtp.gmail.com with ESMTPSA id s21-20020a2e9c15000000b0024917f4e911sm343689lji.80.2022.03.16.22.47.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 16 Mar 2022 22:47:39 -0700 (PDT) Message-ID: <8e836d75-97b0-d301-4d6a-92025e91cad5@redhat.com> Date: Thu, 17 Mar 2022 07:47:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev To: Jason Gunthorpe Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, apopple@nvidia.com, jhubbard@nvidia.com, rcampbell@nvidia.com, vbabka@suse.cz References: <20220311033050.22724-1-mpenttil@redhat.com> <20220314182439.GB64706@ziepe.ca> <20220315183922.GC64706@ziepe.ca> From: =?UTF-8?Q?Mika_Penttil=c3=a4?= In-Reply-To: <20220315183922.GC64706@ziepe.ca> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed X-Rspamd-Queue-Id: C7D1C20016 X-Rspam-User: Authentication-Results: imf31.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=XVwx10HL; spf=none (imf31.hostedemail.com: domain of mpenttil@redhat.com has no SPF policy when checking 170.10.129.124) smtp.mailfrom=mpenttil@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Stat-Signature: 3hefmczsb6u8za86prpw3jusdo98r9t1 X-Rspamd-Server: rspam07 X-HE-Tag: 1647496064-173613 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 15.3.2022 20.39, Jason Gunthorpe wrote: > On Tue, Mar 15, 2022 at 05:22:15AM +0200, Mika Penttil=C3=A4 wrote: >> Hi Jason and thanks for your comments.. >> >> On 14.3.2022 20.24, Jason Gunthorpe wrote: >>> On Fri, Mar 11, 2022 at 05:30:50AM +0200, mpenttil@redhat.com wrote: >>>> From: Mika Penttil=C3=A4 >>>> >>>> HMM selftests use an in-kernel pseudo device to emulate device priva= te >>>> memory. The pseudo device registers a major device range for two pse= udo >>>> device instances. User space has a script that reads /proc/devices i= n >>>> order to find the assigned major number, and sends that to mknod(1), >>>> once for each node. >>>> >>>> This duplicates a fair amount of boilerplate that misc device can do >>>> instead. >>>> >>>> Change this to use misc device, which makes the device node names ap= pear >>>> for us. This also enables udev-like processing if desired. >>> >>> This is borderline the wrong way to use misc devices, they should >>> never be embedded into other structs like this. It works out here >>> because they are eventually only placed in a static array, but still >>> it is a generally bad pattern to see. >> >> Could you elaborate on this one? We have many in-tree usages of the sa= me >> pattern, like: >=20 > The kernel is full of bugs >=20 >> drivers/video/fbdev/pxa3xx-gcu.c >=20 > ie this is broken because it allocates like this: >=20 > priv =3D devm_kzalloc(dev, sizeof(struct pxa3xx_gcu_priv), GFP= _KERNEL); > if (!priv) > return -ENOMEM; >=20 > And free's via devm: >=20 >=20 > static int pxa3xx_gcu_remove(struct platform_device *pdev) > { > struct pxa3xx_gcu_priv *priv =3D platform_get_drvdata(pdev); >=20 > misc_deregister(&priv->misc_dev); > return 0; > } >=20 > But this will UAF if it races fops open with misc_desregister. Yes this driver is broken because platform_device and miscdevice have=20 unrelated lifetimes. >=20 > Proper use of cdevs with proper struct devices prevent this bug. >=20 >> You mention "placed in a static array", are you seeing a potential lif= etime >> issue or what? Many of the examples above embed miscdevice in a dynami= cally >> allocated object also. >> >> The file object's private_data holds a pointer to the miscdevice, and >> fops_get() pins the module. So freeing the objects miscdevice is embed= ded in >> at module_exit time should be fine. But, as you said, in this case the >> miscdevices are statically allocated, so that shouldn't be an issue >> either. >=20 > Correct, it is OK here because the module refcounts prevent the > miscdevice memory from being freed, the above cases with dynamic > allocations do not have that protection and are wrong. >=20 > This is why I don't care for the pattern of putting misc devices > inside other structs, it suggests this is perhaps generally safe but > it is not. >=20 >> I think using cdev_add ends up in the same results in device_* api >> sense. >=20 > Nope, everything works right once you use cdev_device_add on a > properly registered struct device. >=20 >> miscdevice acting like a mux at a higher abstraction level simplifies = the >> code. >=20 > It does avoid the extra struct device, but at the cost of broken > memory lifetime No, misc_register() ends up calling device_create_with_groups() so there=20 is struct device involved. cdev_device_add() would make the explicit=20 struct_device as a parent of the cdev kobj ensuring that struct_device=20 (and maybe structure containing it) is not free before the cdev. But the=20 lifetime of the objects here are controlled by the module lifetime.=20 Note, there is also cdev involved with miscdevice protecting misc.ko and=20 our fops protecting our module unload. I don't mind using the cdev APIs per se, just would like to do for the=20 right reasons. Using cdev apis might be just overkill for many usages,=20 and that's where miscdevice is useful. It miscdevice is broken in some=20 subtle ways I think it should be better documented, or better yet, fixed. >=20 > Jason >=20 Thanks, Mika