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
| ||
|
Date: Thu, 28 Apr 2022 11:20:16 +0200 From: Greg KH <gregkh@...uxfoundation.org> To: Lin Ma <linma@....edu.cn> Cc: Jakub Kicinski <kuba@...nel.org>, Duoming Zhou <duoming@....edu.cn>, krzysztof.kozlowski@...aro.org, pabeni@...hat.com, linux-kernel@...r.kernel.org, davem@...emloft.net, alexander.deucher@....com, akpm@...ux-foundation.org, broonie@...nel.org, netdev@...r.kernel.org Subject: Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able On Thu, Apr 28, 2022 at 04:49:18PM +0800, Lin Ma wrote: > Hello Greg, > > > > > It shouldn't be, if you are using it properly :) > > > > [...] > > > > Yes, you should almost never use that call. Seems the nfc subsystem is > > the most common user of it for some reason :( > > Cool, and I believe that the current nfc core code does not use it properly. :( > > > > > What state are you trying to track here exactly? > > > > Forget about the firmware downloading race that raised by Duoming in this channel, > all the netlink handler code in net/nfc/core.c depends on the device_is_registered > macro. > > My idea is to introduce a patch like below: > > include/net/nfc/nfc.h | 1 + > net/nfc/core.c | 26 ++++++++++++++------------ > 2 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/include/net/nfc/nfc.h b/include/net/nfc/nfc.h > index 5dee575fbe86..d84e53802b06 100644 > --- a/include/net/nfc/nfc.h > +++ b/include/net/nfc/nfc.h > @@ -168,6 +168,7 @@ struct nfc_dev { > int targets_generation; > struct device dev; > bool dev_up; > + bool dev_register; > bool fw_download_in_progress; > u8 rf_mode; > bool polling; > diff --git a/net/nfc/core.c b/net/nfc/core.c > index dc7a2404efdf..208e6bb0804e 100644 > --- a/net/nfc/core.c > +++ b/net/nfc/core.c > @@ -38,7 +38,7 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name) > > device_lock(&dev->dev); > > - if (!device_is_registered(&dev->dev)) { > + if (!dev->dev_register) { > rc = -ENODEV; > goto error; > } > @@ -94,7 +94,7 @@ int nfc_dev_up(struct nfc_dev *dev) > > device_lock(&dev->dev); > > - if (!device_is_registered(&dev->dev)) { > + if (!dev->dev_register) { > rc = -ENODEV; > goto error; > } > > [...] > > @@ -1134,6 +1134,7 @@ int nfc_register_device(struct nfc_dev *dev) > dev->rfkill = NULL; > } > } > + dev->dev_register = true; > device_unlock(&dev->dev); > > rc = nfc_genl_device_added(dev); > @@ -1162,6 +1163,7 @@ void nfc_unregister_device(struct nfc_dev *dev) > "was removed\n", dev_name(&dev->dev)); > > device_lock(&dev->dev); > + dev->dev_register = false; > if (dev->rfkill) { > rfkill_unregister(dev->rfkill); > rfkill_destroy(dev->rfkill); > -- > 2.35.1 > > The added dev_register variable can function like the original device_is_registered and does not race-able > because of the protection of device_lock. Yes, that looks better, but what is the root problem here that you are trying to solve? Why does NFC need this when no other subsystem does? thansk, greg k-h
Powered by blists - more mailing lists