Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
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
Content-Language: en-US
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
References: <20220311033050.22724-1-mpenttil@redhat.com>
 <20220314182439.GB64706@ziepe.ca>
 <d9b9c82e-4012-cf0a-d966-d9669a684a27@redhat.com>
 <20220315183922.GC64706@ziepe.ca>
From:   =?UTF-8?Q?Mika_Penttil=c3=a4?= <mpenttil@redhat.com>
In-Reply-To: <20220315183922.GC64706@ziepe.ca>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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