netdev - Re: [PATCH v12 01/10] i915: only initialize struct ref_tracker

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0ae02bf15487f3e5703ce1f5d3107b6dc00477f1.camel@kernel.org>
Date: Fri, 30 May 2025 07:39:36 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Krzysztof Karas <krzysztof.karas@...el.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, "David S. Miller"	
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski	
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman	
 <horms@...nel.org>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann
 <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,  Simona Vetter
 <simona@...ll.ch>, Jani Nikula <jani.nikula@...ux.intel.com>, Joonas
 Lahtinen	 <joonas.lahtinen@...ux.intel.com>, Rodrigo Vivi
 <rodrigo.vivi@...el.com>,  Tvrtko Ursulin <tursulin@...ulin.net>, Kuniyuki
 Iwashima <kuniyu@...zon.com>, Qasim Ijaz <qasdev00@...il.com>,  Nathan
 Chancellor	 <nathan@...nel.org>, Andrew Lunn <andrew@...n.ch>,
 linux-kernel@...r.kernel.org, 	netdev@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, 	intel-gfx@...ts.freedesktop.org
Subject: Re: [PATCH v12 01/10] i915: only initialize struct ref_tracker_dir
 once

On Fri, 2025-05-30 at 11:01 +0000, Krzysztof Karas wrote:
> Hi Jeff,
> 
> > I got some warnings from the i915 CI with the ref_tracker debugfs
> > patches applied, that indicated that these ref_tracker_dir_init() calls
> > were being called more than once. If references were held on these
> > objects between the initializations, then that could lead to leaked ref
> > tracking objects.
> > 
> > Since these objects are zalloc'ed, ensure that they are only initialized
> > once by testing whether the first byte of the name field is 0.
> 
> Are you referring to these warnings?
> <3> [314.043410] debugfs: File 'intel_wakeref@...f88815111a308' in directory 'ref_tracker' already present!
> <4> [314.043427] ref_tracker: ref_tracker: unable to create debugfs file for intel_wakeref@...f88815111a308: -EEXIST
> 
> I think those might be caused by introduction of:
> "ref_tracker: automatically register a file in debugfs for a ref_tracker_dir".
> 
> Current version of "ref_tracker: add a static classname string
> to each ref_tracker_dir" further in this series should prevent
> multiple calls to "ref_tracker_dir_init()", so this patch could
> be dropped I think.
> If my reasoning is wrong however, then please add a note to the
> commit message which explains why this is needed in more detail
> or/and move this patch right before it is necessary. Otherwise
> it looks like a vague workaround.
> 

I'm fine with dropping this patch.

Those are the messages that demonstrate the problem, but the problem is
potentially bigger than those messages. ref_tracker_dir_init() is being
called (at least) twice:

struct ref_tracker_dir {                                              
#ifdef CONFIG_REF_TRACKER                                             
        spinlock_t              lock;                                 
        unsigned int            quarantine_avail;                     
        refcount_t              untracked;                            
        refcount_t              no_tracker;                           
        bool                    dead;                                 
        struct list_head        list; /* List of active trackers */   
        struct list_head        quarantine; /* List of dead trackers */
        const char              *class; /* object classname */        
#ifdef CONFIG_DEBUG_FS                                                
        struct dentry           *dentry;                              
        struct dentry           *symlink;                             
#endif                                                                
#endif                                                                
}; 

This structure contains two list_heads that can contain ref_tracker
objects. If that list was populated when ref_tracker_dir_init() is
called the second time, then those objects will now be sitting on a
corrupt list. At best they'll just leak, but with them sitting on a
now-corrupt list, they could cause a panic too.

It may be that there can be no objects on that list when it's called
the second time. But with this patchset initializing it twice will
cause dentry leaks at least.
-- 
Jeff Layton <jlayton@...nel.org>