linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Mika Penttilä" <mpenttil@redhat.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	apopple@nvidia.com, jhubbard@nvidia.com, rcampbell@nvidia.com,
	vbabka@suse.cz
Subject: Re: [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev
Date: Thu, 17 Mar 2022 07:47:38 +0200	[thread overview]
Message-ID: <8e836d75-97b0-d301-4d6a-92025e91cad5@redhat.com> (raw)
In-Reply-To: <20220315183922.GC64706@ziepe.ca>



On 15.3.2022 20.39, Jason Gunthorpe wrote:
> On Tue, Mar 15, 2022 at 05:22:15AM +0200, Mika Penttilä 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ä <mpenttil@redhat.com>
>>>>
>>>> HMM selftests use an in-kernel pseudo device to emulate device private
>>>> memory. The pseudo device registers a major device range for two pseudo
>>>> device instances. User space has a script that reads /proc/devices in
>>>> 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 appear
>>>> 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 same
>> pattern, like:
> 
> The kernel is full of bugs
> 
>> drivers/video/fbdev/pxa3xx-gcu.c
> 
> ie this is broken because it allocates like this:
> 
>          priv = devm_kzalloc(dev, sizeof(struct pxa3xx_gcu_priv), GFP_KERNEL);
>          if (!priv)
>                  return -ENOMEM;
> 
> And free's via devm:
> 
> 
> static int pxa3xx_gcu_remove(struct platform_device *pdev)
> {
>          struct pxa3xx_gcu_priv *priv = platform_get_drvdata(pdev);
> 
>          misc_deregister(&priv->misc_dev);
>          return 0;
> }
> 
> But this will UAF if it races fops open with misc_desregister.


Yes this driver is broken because platform_device and miscdevice have 
unrelated lifetimes.

> 
> Proper use of cdevs with proper struct devices prevent this bug.
> 
>> You mention "placed in a static array", are you seeing a potential lifetime
>> issue or what? Many of the examples above embed miscdevice in a dynamically
>> 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 embedded 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.
> 
> 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.
> 
> 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.
> 
>> I think using cdev_add ends up in the same results in device_* api
>> sense.
> 
> Nope, everything works right once you use cdev_device_add on a
> properly registered struct device.
> 
>> miscdevice acting like a mux at a higher abstraction level simplifies the
>> code.
> 
> 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 
is struct device involved. cdev_device_add() would make the explicit 
struct_device as a parent of the cdev kobj ensuring that struct_device 
(and maybe structure containing it) is not free before the cdev. But the 
lifetime of the objects here are controlled by the module lifetime. 
Note, there is also cdev involved with miscdevice protecting misc.ko and 
our fops protecting our module unload.

I don't mind using the cdev APIs per se, just would like to do for the 
right reasons. Using cdev apis might be just overkill for many usages, 
and that's where miscdevice is useful. It miscdevice is broken in some 
subtle ways I think it should be better documented, or better yet, fixed.


> 
> Jason
> 


Thanks,
Mika


  reply	other threads:[~2022-03-17  6:08 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-11  3:30 [PATCH v2] mm/hmm/test: simplify hmm test code: use miscdevice instead of char dev mpenttil
2022-03-11  6:15 ` John Hubbard
2022-03-11  6:18   ` Mika Penttilä
2022-03-14 18:24 ` Jason Gunthorpe
2022-03-15  3:22   ` Mika Penttilä
2022-03-15 18:39     ` Jason Gunthorpe
2022-03-17  5:47       ` Mika Penttilä [this message]
2022-03-17  6:58         ` Mika Penttilä
2022-03-17 14:15           ` Jason Gunthorpe
2022-03-18  2:34             ` Mika Penttilä
2022-03-18  2:58               ` John Hubbard
2022-03-18 12:40                 ` Jason Gunthorpe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8e836d75-97b0-d301-4d6a-92025e91cad5@redhat.com \
    --to=mpenttil@redhat.com \
    --cc=apopple@nvidia.com \
    --cc=jgg@ziepe.ca \
    --cc=jhubbard@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=rcampbell@nvidia.com \
    --cc=vbabka@suse.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).