Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack

[Michel Dänzer]

On 3/4/26 13:32, Julian Orth wrote:
> On Wed, Mar 4, 2026 at 12:47 PM Michel Dänzer
> <michel.daenzer@xxxxxxxxxxx> wrote:
>> On 3/4/26 12:25, Julian Orth wrote:
>>> On Wed, Mar 4, 2026 at 12:15 PM Michel Dänzer
>>> <michel.daenzer@xxxxxxxxxxx> wrote:
>>>> On 3/3/26 20:12, Julian Orth wrote:
>>>>> On Tue, Mar 3, 2026 at 7:53 PM Michel Dänzer <michel.daenzer@xxxxxxxxxxx> wrote:
>>>>>> On 3/3/26 18:44, Maarten Lankhorst wrote:
>>>>>>>
>>>>>>> You don't even need to use memset, this would work too:
>>>>>>>
>>>>>>> struct drm_syncobj_handle args = {
>>>>>>>       .flags = 0
>>>>>>> };
>>>>>>
>>>>>> TL;DR: This method isn't 100% safe either.
>>>>>>
>>>>>> It won't initialize any padding which isn't covered by any struct field. We try to avoid that and have explicit padding fields instead, mistakes may happen though, and in theory such padding could later be used for a new field.
>>>>>
>>>>> I don't think this is workable.
>>>>
>>>> libdrm begs to differ. It shows that it's not only workable but really easy. There's no reason for doing it any other way.
>>>
>>> Using memset to initialize padding bytes between fields is workable.
>>> Having the kernel add checks for this for existing ioctls is not
>>> workable because it would break usespace that doesn't do this.
>>
>> As discussed in this thread, memset is also required for when the size of an ioctl struct is extended, even if there is no such padding.
> 
> That is not a concern in rust code. If the rust code extends the
> definition of the struct, all call sites will cause a compilation
> error until the new field is also initialized.
> 
> The issue I'm talking about here is strictly about padding bytes between fields.

So this is not related to the issue which motivated your patch?


>>> Which is every rust program out there as far as I can tell.
>>
>> That's surprising. Surely there must be some unsafe code involved which allows uninitialized memory to be passed to ioctl()?
> 
> The memory is not uninitialized from the perspective of the rust
> language since all fields are initialized. Only the padding bytes are
> uninitialized and they cannot be accessed in safe rust, therefore no
> unsafe is required.
> 
> I've never seen any rust code that uses memset to initialize ioctl
> arguments. It assumes that the ioctl implementation will only read
> from the named fields. Therefore, while the ioctl syscall itself is
> unsafe, developers assume that the safety requirements are satisfied
> in this regard.

As discussed in this thread, that's not a valid assumption and may blow up in their face sooner or later. ("No padding not covered by any fields" is best-effort, not a guarantee)


-- 
Earthling Michel Dänzer       \        GNOME / Xwayland / Mesa developer
https://redhat.com             \               Libre software enthusiast