netdev - Re: [PATCH net v2 2/6] netdevsim: disable devlink reload when resources are being used

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

Date:   Sun, 2 Feb 2020 21:14:19 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     David Miller <davem@...emloft.net>, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2 2/6] netdevsim: disable devlink reload when
 resources are being used

On Sun, 2 Feb 2020 at 02:23, Jakub Kicinski <kuba@...nel.org> wrote:
>

Hi Jakub,

> On Sat, 1 Feb 2020 18:37:58 +0900, Taehee Yoo wrote:
> > > > +     mutex_lock(&nsim_bus_dev_ops_lock);
> > >
> > > Not sure why we have to lock the big lock here?
> >
> > The reason for using this lock is to protect "nsim_dev".
> > nsim_dev_take_snapshot_write() uses nsim_dev.
> > So if nsim_dev is removed while this function is being used,
> > panic will occur.
> > nsim_dev is protected by nsim_bus_dev_ops_lock.
> > So, this lock should be used.
>
> I see.
>
> > But, I found deadlock because of this lock.
> > Structurally, this lock couldn't be used in snapshot_write().
> > So, I will find another way.
>
> Could we perhaps use the lock in struct device? Seems like it would
> be a good fit for protecting nsim_dev?

Thank you for your suggestion!

There is a lock in the struct device, which is "mutex".
This lock is used by wrapper function "device_lock()".
I think this lock is usually used for protecting members of the struct
device in device driver core logic.
And I think that both variables "nsim_dev" and "nsim_dev->dummy_region"
are not the actual member of the struct device.
nsim_dev and nsim_dummy_region would be allocated and freed independently
of struct device. So, I don't know device_lock() is fit for protecting
nsim_dev and nsim_dev->dummy_region.

I already sent a v3 patchset, which has a patch to avoid this problem.
The way of this is to use internal debugfs synchronize routine.
debugfs_remove() internally waits for opened users.
So, If we remove debugfs file before removing nsim_dev and
nsim_dev->dummy_region, the use-after-free case will not occur.
And it doesn't need an additional lock. So the performance would be
a little bit better.

Thank you!
Taehee Yoo