linux-kernel - Re: [PATCH] module: print module name on refcount error

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: <20230828141830.02de4d90@endymion.delvare>
Date:   Mon, 28 Aug 2023 14:18:30 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     Michal Hocko <mhocko@...e.com>, linux-modules@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] module: print module name on refcount error

Hi Luis, Michal, 

On Wed, 26 Jul 2023 13:59:06 -0700, Luis Chamberlain wrote:
> On Mon, Jul 10, 2023 at 07:43:01AM +0200, Michal Hocko wrote:
> > On Fri 07-07-23 11:56:49, Luis Chamberlain wrote:  
> > > On Mon, Jul 03, 2023 at 03:47:22PM +0200, Michal Hocko wrote:  
> > > > On Fri 30-06-23 16:05:33, Luis Chamberlain wrote:
> > > > [...]  
> > > > > What prevents code from racing the free with a random module_put()
> > > > > called by some other piece of code?  
> > > > 
> > > > Wouldn't be ref count a garbage already? How can you race when freeing
> > > > if module_put fail?  
> > > 
> > > It could yes, ie, so this risks at all being junk.  
> > 
> > Could you be more specific please? I still do not see a scenario where
> > module string name would be junk while refcount itself would be a valid
> > memory.  
> 
> That is true, but if refcount is invalid so will the memory for the
> string.

This isn't how I read the code, and this is exactly the reason why I
submitted this patch in the first place.

As far as I can see, there are 3 possibilities:

1* The refcount is correct, everything is fine.
2* The refcount is wrong (we are trying to put a ref which was never
   taken), however the module wasn't unloaded yet, so the module name is
   still readable.
3* The refcount is wrong and the module has already been unloaded. The
   memory may have been reused already, so the module name can't be read.

My patch is only useful in case 2. Although it doesn't cover all cases,
I think it is relevant because unloading modules is something you
rarely do in production, so if the refcount goes wrong, we will almost
always be in case 2.

That being said, if you don't like my proposal for whatever reason, or
prefer addressing the issue in a different way, no problem at all.

> > It would likely be better to use refcount_t instead of atomic_t.  
> 
> Patches welcomed.

Michal, do I understand correctly that this would prevent the case our
customer had (too many gets), but won't make a difference for actual
too-many-puts situations?

-- 
Jean Delvare
SUSE L3 Support