Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

[Al Viro]

On Thu, May 01, 2025 at 07:13:12PM -0700, Matthew Brost wrote:
> On Thu, May 01, 2025 at 05:24:38PM -0700, Kees Cook wrote:
> > Casting through a "void *" isn't sufficient to convince the randstruct
> > GCC plugin that the result is intentional. Instead operate through an
> > explicit union to silence the warning:
> > 
> > drivers/gpu/drm/ttm/ttm_backup.c: In function 'ttm_file_to_backup':
> > drivers/gpu/drm/ttm/ttm_backup.c:21:16: note: randstruct: casting between randomized structure pointer types (ssa): 'struct ttm_backup' and 'struct file'
> >    21 |         return (void *)file;
> >       |                ^~~~~~~~~~~~
> > 
> > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> 
> I forgot the policy if suggest-by but will add:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> Thomas was out today I suspect he will look at this tomorrow when he is
> back too.

[fsdevel and the rest of VFS maintainers Cc'd]

NAKed-by: Al Viro <viro@zeniv.linux.org.uk>

Reason: struct file should *NOT* be embedded into other objects without
the core VFS being very explicitly aware of those.  The only such case
is outright local to fs/file_table.c, and breeding more of those is
a really bad idea.

Don't do that.  Same goes for struct {dentry, super_block, mount}
in case anyone gets similar ideas.

Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

[Kees Cook]

On Fri, May 02, 2025 at 03:34:47AM +0100, Al Viro wrote:
> On Thu, May 01, 2025 at 07:13:12PM -0700, Matthew Brost wrote:
> > On Thu, May 01, 2025 at 05:24:38PM -0700, Kees Cook wrote:
> > > Casting through a "void *" isn't sufficient to convince the randstruct
> > > GCC plugin that the result is intentional. Instead operate through an
> > > explicit union to silence the warning:
> > > 
> > > drivers/gpu/drm/ttm/ttm_backup.c: In function 'ttm_file_to_backup':
> > > drivers/gpu/drm/ttm/ttm_backup.c:21:16: note: randstruct: casting between randomized structure pointer types (ssa): 'struct ttm_backup' and 'struct file'
> > >    21 |         return (void *)file;
> > >       |                ^~~~~~~~~~~~
> > > 
> > > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > I forgot the policy if suggest-by but will add:
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > Thomas was out today I suspect he will look at this tomorrow when he is
> > back too.
> 
> [fsdevel and the rest of VFS maintainers Cc'd]
> 
> NAKed-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> Reason: struct file should *NOT* be embedded into other objects without
> the core VFS being very explicitly aware of those.  The only such case
> is outright local to fs/file_table.c, and breeding more of those is
> a really bad idea.
> 
> Don't do that.  Same goes for struct {dentry, super_block, mount}
> in case anyone gets similar ideas.

Well, in that case, the NAK should be against commit e7b5d23e5d47
("drm/ttm: Provide a shmem backup implementation"), but that looks to
have had 15 revisions already...

But I will just back away slowly now. I was just fixing a warning
introduced by it. :)

-- 
Kees Cook

Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

[Matthew Brost]

On Fri, May 02, 2025 at 03:34:47AM +0100, Al Viro wrote:
> On Thu, May 01, 2025 at 07:13:12PM -0700, Matthew Brost wrote:
> > On Thu, May 01, 2025 at 05:24:38PM -0700, Kees Cook wrote:
> > > Casting through a "void *" isn't sufficient to convince the randstruct
> > > GCC plugin that the result is intentional. Instead operate through an
> > > explicit union to silence the warning:
> > > 
> > > drivers/gpu/drm/ttm/ttm_backup.c: In function 'ttm_file_to_backup':
> > > drivers/gpu/drm/ttm/ttm_backup.c:21:16: note: randstruct: casting between randomized structure pointer types (ssa): 'struct ttm_backup' and 'struct file'
> > >    21 |         return (void *)file;
> > >       |                ^~~~~~~~~~~~
> > > 
> > > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > I forgot the policy if suggest-by but will add:
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > 
> > Thomas was out today I suspect he will look at this tomorrow when he is
> > back too.
> 
> [fsdevel and the rest of VFS maintainers Cc'd]
> 
> NAKed-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> Reason: struct file should *NOT* be embedded into other objects without

I;m fairly certain is just aliasing... but I do understand a file cannot
be embedded. Would comment help here indicating no other fields should
be added to ttm_backup without struct file be converted to pointer or
that just to risky?

Matt

> the core VFS being very explicitly aware of those.  The only such case
> is outright local to fs/file_table.c, and breeding more of those is
> a really bad idea.
> 
> Don't do that.  Same goes for struct {dentry, super_block, mount}
> in case anyone gets similar ideas.

Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

[Al Viro]

On Thu, May 01, 2025 at 09:26:25PM -0700, Matthew Brost wrote:

> I;m fairly certain is just aliasing... but I do understand a file cannot
> be embedded. Would comment help here indicating no other fields should
> be added to ttm_backup without struct file be converted to pointer or
> that just to risky?

What exactly are you trying to do there?  IOW, is that always supposed to
be a struct file, or something dependent upon something in struct ttm_tt
instance, or...?

And what is the lifecycle of that thing?  E.g. what is guaranteed about
ttm_backup_fini() vs. functions accessing the damn thing?  Are they
serialized on something/tied to lifecycle stages of struct ttm_tt?

Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

[Matthew Brost]

On Fri, May 02, 2025 at 05:31:49AM +0100, Al Viro wrote:
> On Thu, May 01, 2025 at 09:26:25PM -0700, Matthew Brost wrote:
> 
> > I;m fairly certain is just aliasing... but I do understand a file cannot
> > be embedded. Would comment help here indicating no other fields should
> > be added to ttm_backup without struct file be converted to pointer or
> > that just to risky?
> 
> What exactly are you trying to do there?  IOW, is that always supposed to
> be a struct file, or something dependent upon something in struct ttm_tt
> instance, or...?

Create an opaque ttm_backup object for the rest of TTM / drivers to view
- it could change if the backup implementation changed.

> 
> And what is the lifecycle of that thing?  E.g. what is guaranteed about
> ttm_backup_fini() vs. functions accessing the damn thing?  Are they
> serialized on something/tied to lifecycle stages of struct ttm_tt?

I believe the life cycle is when ttm_tt is destroyed or api allows
overriding the old backup with a new one (currently unused).

Matt

Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

[Al Viro]

On Thu, May 01, 2025 at 09:52:08PM -0700, Matthew Brost wrote:
> On Fri, May 02, 2025 at 05:31:49AM +0100, Al Viro wrote:
> > On Thu, May 01, 2025 at 09:26:25PM -0700, Matthew Brost wrote:

> > And what is the lifecycle of that thing?  E.g. what is guaranteed about
> > ttm_backup_fini() vs. functions accessing the damn thing?  Are they
> > serialized on something/tied to lifecycle stages of struct ttm_tt?
> 
> I believe the life cycle is when ttm_tt is destroyed or api allows
> overriding the old backup with a new one (currently unused).

Umm...  So can ttm_tt_setup_backup() be called in the middle of
e.g. ttm_backup_drop() or ttm_backup_{copy,backup}_page(), etc.?

I mean, if they had been called by ttm_backup.c internals, it would
be an internal business of specific implementation, with all
serialization, etc. warranties being its responsibility;
but if it's called by other code that is supposed to be isolated
from details of what ->backup is pointing to...

Sorry for asking dumb questions, but I hadn't seen the original
threads.  Basically, what prevents the underlying shmem file getting
torn apart while another operation is using it?  It might very well
be simple, but I had enough "it's because of... oh, bugger" moments
on the receiving end of such questions...

Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

[Christian König]

On 5/2/25 07:33, Al Viro wrote:
> On Thu, May 01, 2025 at 09:52:08PM -0700, Matthew Brost wrote:
>> On Fri, May 02, 2025 at 05:31:49AM +0100, Al Viro wrote:
>>> On Thu, May 01, 2025 at 09:26:25PM -0700, Matthew Brost wrote:
> 
>>> And what is the lifecycle of that thing?  E.g. what is guaranteed about
>>> ttm_backup_fini() vs. functions accessing the damn thing?  Are they
>>> serialized on something/tied to lifecycle stages of struct ttm_tt?
>>
>> I believe the life cycle is when ttm_tt is destroyed or api allows
>> overriding the old backup with a new one (currently unused).
> 
> Umm...  So can ttm_tt_setup_backup() be called in the middle of
> e.g. ttm_backup_drop() or ttm_backup_{copy,backup}_page(), etc.?
> 
> I mean, if they had been called by ttm_backup.c internals, it would
> be an internal business of specific implementation, with all
> serialization, etc. warranties being its responsibility;
> but if it's called by other code that is supposed to be isolated
> from details of what ->backup is pointing to...
> 
> Sorry for asking dumb questions, but I hadn't seen the original
> threads.  Basically, what prevents the underlying shmem file getting
> torn apart while another operation is using it?  It might very well
> be simple, but I had enough "it's because of... oh, bugger" moments
> on the receiving end of such questions...

It's the outside logic which makes sure that the backup structure stays around as long as the BO or the device which needs it is around.

But putting that aside I was not very keen about the whole idea of never defining the ttm_backup structure and just casting it to a file in the backend either.

So I would just completely nuke that unnecessary abstraction and just use a pointer to a file all around.

Regards,
Christian.

Re: [PATCH v2] drm/ttm: Silence randstruct warning about casting struct file

[Thomas Hellström]

On Fri, 2025-05-02 at 09:49 +0200, Christian König wrote:
> On 5/2/25 07:33, Al Viro wrote:
> > On Thu, May 01, 2025 at 09:52:08PM -0700, Matthew Brost wrote:
> > > On Fri, May 02, 2025 at 05:31:49AM +0100, Al Viro wrote:
> > > > On Thu, May 01, 2025 at 09:26:25PM -0700, Matthew Brost wrote:
> > 
> > > > And what is the lifecycle of that thing?  E.g. what is
> > > > guaranteed about
> > > > ttm_backup_fini() vs. functions accessing the damn thing?  Are
> > > > they
> > > > serialized on something/tied to lifecycle stages of struct
> > > > ttm_tt?
> > > 
> > > I believe the life cycle is when ttm_tt is destroyed or api
> > > allows
> > > overriding the old backup with a new one (currently unused).
> > 
> > Umm...  So can ttm_tt_setup_backup() be called in the middle of
> > e.g. ttm_backup_drop() or ttm_backup_{copy,backup}_page(), etc.?
> > 
> > I mean, if they had been called by ttm_backup.c internals, it would
> > be an internal business of specific implementation, with all
> > serialization, etc. warranties being its responsibility;
> > but if it's called by other code that is supposed to be isolated
> > from details of what ->backup is pointing to...
> > 
> > Sorry for asking dumb questions, but I hadn't seen the original
> > threads.  Basically, what prevents the underlying shmem file
> > getting
> > torn apart while another operation is using it?  It might very well
> > be simple, but I had enough "it's because of... oh, bugger" moments
> > on the receiving end of such questions...
> 
> It's the outside logic which makes sure that the backup structure
> stays around as long as the BO or the device which needs it is
> around.
> 
> But putting that aside I was not very keen about the whole idea of
> never defining the ttm_backup structure and just casting it to a file
> in the backend either.
> 
> So I would just completely nuke that unnecessary abstraction and just
> use a pointer to a file all around.

Hmm, yes there were early the series a number of different
implementations of the struct ttm_backup. Initially because previous
attempts of using a shmem object failed but now that we've landed on a
shmem object, We should be able to replace it with a struct file
pointer.

Let me take a look at this. 

/Thomas

> 
> Regards,
> Christian.