linux-kernel - Re: [RFC PATCH] fpga: remove module reference counting from core components

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: <2023110922-lurk-subdivide-4962@gregkh>
Date:   Thu, 9 Nov 2023 06:27:24 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Xu Yilun <yilun.xu@...ux.intel.com>
Cc:     Marco Pagani <marpagan@...hat.com>,
        Moritz Fischer <mdf@...nel.org>, Wu Hao <hao.wu@...el.com>,
        Xu Yilun <yilun.xu@...el.com>, Tom Rix <trix@...hat.com>,
        Alan Tull <atull@...nsource.altera.com>,
        linux-fpga@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] fpga: remove module reference counting from core
 components

On Thu, Nov 09, 2023 at 01:07:42PM +0800, Xu Yilun wrote:
> On Wed, Nov 08, 2023 at 05:20:53PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Nov 08, 2023 at 11:52:52PM +0800, Xu Yilun wrote:
> > > > >>
> > > > >> In fpga_region_get() / fpga_region_put(): call get_device() before
> > > > >> acquiring the mutex and put_device() after having released the mutex
> > > > >> to avoid races.
> > 
> > Why do you need another reference count with a lock?  You already have
> > that with the calls to get/put_device().
> 
> The low-level driver module could still be possibly unloaded at the same
> time, if so, when FPGA core run some callbacks provided by low-level driver
> module, its referenced page of code is unmapped...

Then something is designed wrong here, the unloading of the low-level
driver should remove the access to the device itself.  Perhaps fix that?

> > > The lock/unlock of mutex
> > > scattered in different functions is also an uncommon style. Maybe some
> > > atomic count should be enough.
> > 
> > Don't make a fake lock with an atomic variable, use real locks if you
> > need it.
> 
> I mean, here it doesn't not looks like a "locking" senario, although it
> works.
> 
> The purpose here is to declare a device state, which says the device is
> exclusively used by a user, no other user is allowed. But usually we use
> mutex to protect against critical code blocks, not to represent a long
> live device state.
> 
> I'm still OK for the existing mutex usage as it doesn't break anything
> and if we don't want change much.
> 
> > 
> > Or don't even care about module unloading at all!  Why does it matter?
> > It can never happen without explicit intervention and it's something
> > that a lot of the time, just will cause problems.  Don't do a lot of
> > extra work for something that doesn't matter.
> 
> mm.. as mentioned above some fundamental subsystems do care about
> module unloading, and I tend to keep it same way. But mm... I'm OK to
> make things easier if you insist.

You can care, but my point being that don't make it very complex and
slow just for something that no one will ever do in normal device
operation.

thanks,

greg k-h