linux-kernel - Re: [PATCH v2 1/9] drm/vc4: Switch to container_of

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9d5346bf-526d-d8ec-d055-175aae429638@xs4all.nl>
Date:   Tue, 21 Feb 2023 15:18:34 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     Emma Anholt <emma@...olt.net>, David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Thomas Zimmermann <tzimmermann@...e.de>
Subject: Re: [PATCH v2 1/9] drm/vc4: Switch to container_of_const

On 2/21/23 12:38, Maxime Ripard wrote:
> Hi Hans,
> 
> On Sat, Feb 18, 2023 at 11:45:04AM +0100, Hans Verkuil wrote:
>>> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
>>> index 86d629e45307..d0a00ed42cb0 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_bo.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
>>> @@ -609,7 +609,7 @@ static void vc4_free_object(struct drm_gem_object *gem_bo)
>>>  static void vc4_bo_cache_time_work(struct work_struct *work)
>>>  {
>>>  	struct vc4_dev *vc4 =
>>> -		container_of(work, struct vc4_dev, bo_cache.time_work);
>>> +		container_of_const(work, struct vc4_dev, bo_cache.time_work);
>>
>> ...I think this is misleading. It's definitely not const, so switching to
>> container_of_const suggests that there is some 'constness' involved, which
>> isn't the case. I'd leave this just as 'container_of'. This also reduces the
>> size of the patch, since this is done in quite a few places.
> 
> The name threw me off too, but it's supposed to keep the argument
> pointer constness, not always take and return a const pointer. I still
> believe that it's beneficial since, if the work pointer was ever to
> change constness, we would have that additional check.

If both inner (work) and outer (vc4) pointers are non-const, then there is no sense
in switching to container_of_const. I don't see it used like that elsewhere
in the kernel.

It only makes sense to use it if the inner pointer might be const.

If the work pointer (in this example) would ever become const, then the
regular container_of macro would report a warning, that's something that
was added in commit 7376e561fd2e. So preemptively switching to container_of_const
appears unnecessary to me.

Regards,

	Hans