On 29/04/2022 03:14, Duoming Zhou wrote:
> The device_is_registered() in nfc core is used to check whether
> nfc device is registered in netlink related functions such as
> nfc_fw_download(), nfc_dev_up() and so on. Although device_is_registered()
> is protected by device_lock, there is still a race condition between
> device_del() and device_is_registered(). The root cause is that
> kobject_del() in device_del() is not protected by device_lock.
>
> (cleanup task) | (netlink task)
> |
> nfc_unregister_device | nfc_fw_download
> device_del | device_lock
> ... | if (!device_is_registered)//(1)
> kobject_del//(2) | ...
> ... | device_unlock
>
> The device_is_registered() returns the value of state_in_sysfs and
> the state_in_sysfs is set to zero in kobject_del(). If we pass check in
> position (1), then set zero in position (2). As a result, the check
> in position (1) is useless.
>
> This patch uses bool variable instead of device_is_registered() to judge
> whether the nfc device is registered, which is well synchronized.
>
> Fixes: 3e256b8f8dfa ("NFC: add nfc subsystem core")
> Signed-off-by: Duoming Zhou <[email protected]>
> ---
> Changes in v5:
> - Replace device_is_registered() to bool variable.
>
> 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 5dee575fbe8..7bb4ccb1830 100644
> --- a/include/net/nfc/nfc.h
> +++ b/include/net/nfc/nfc.h
> @@ -167,6 +167,7 @@ struct nfc_dev {
> int n_targets;
> int targets_generation;
> struct device dev;
> + bool dev_register;
> bool dev_up;
> bool fw_download_in_progress;
> u8 rf_mode;
> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index dc7a2404efd..52147da2286 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;
> }
> @@ -142,7 +142,7 @@ int nfc_dev_down(struct nfc_dev *dev)
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> goto error;
> }
> @@ -207,7 +207,7 @@ int nfc_start_poll(struct nfc_dev *dev, u32 im_protocols, u32 tm_protocols)
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> goto error;
> }
> @@ -246,7 +246,7 @@ int nfc_stop_poll(struct nfc_dev *dev)
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> goto error;
> }
> @@ -291,7 +291,7 @@ int nfc_dep_link_up(struct nfc_dev *dev, int target_index, u8 comm_mode)
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> goto error;
> }
> @@ -335,7 +335,7 @@ int nfc_dep_link_down(struct nfc_dev *dev)
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> goto error;
> }
> @@ -401,7 +401,7 @@ int nfc_activate_target(struct nfc_dev *dev, u32 target_idx, u32 protocol)
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> goto error;
> }
> @@ -448,7 +448,7 @@ int nfc_deactivate_target(struct nfc_dev *dev, u32 target_idx, u8 mode)
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> goto error;
> }
> @@ -495,7 +495,7 @@ int nfc_data_exchange(struct nfc_dev *dev, u32 target_idx, struct sk_buff *skb,
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> kfree_skb(skb);
> goto error;
> @@ -552,7 +552,7 @@ int nfc_enable_se(struct nfc_dev *dev, u32 se_idx)
>
> device_lock(&dev->dev);
>
> - if (!device_is_registered(&dev->dev)) {
> + if (!dev->dev_register) {
> rc = -ENODEV;
> goto error;
> }
> @@ -601,7 +601,7 @@ int nfc_disable_se(struct nfc_dev *dev, u32 se_idx)
>
> 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);
> @@ -1166,6 +1167,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
> rfkill_unregister(dev->rfkill);
> rfkill_destroy(dev->rfkill);
> }
> + dev->dev_register = false;
We already have flag for it - dev->shutting_down. Currently it is used
only in if device implements check_presence but I think it can be easily
moved to common path.
Having multiple fields for similar, but slightly different cases, is
getting us closer and closer to spaghetti code.
Best regards,
Krzysztof
Hello,
On Fri, 29 Apr 2022 09:19:36 +0200 Krzysztof wrote:
> > The device_is_registered() in nfc core is used to check whether
> > nfc device is registered in netlink related functions such as
> > nfc_fw_download(), nfc_dev_up() and so on. Although device_is_registered()
> > is protected by device_lock, there is still a race condition between
> > device_del() and device_is_registered(). The root cause is that
> > kobject_del() in device_del() is not protected by device_lock.
> >
> > (cleanup task) | (netlink task)
> > |
> > nfc_unregister_device | nfc_fw_download
> > device_del | device_lock
> > ... | if (!device_is_registered)//(1)
> > kobject_del//(2) | ...
> > ... | device_unlock
> >
> > The device_is_registered() returns the value of state_in_sysfs and
> > the state_in_sysfs is set to zero in kobject_del(). If we pass check in
> > position (1), then set zero in position (2). As a result, the check
> > in position (1) is useless.
> >
> > This patch uses bool variable instead of device_is_registered() to judge
> > whether the nfc device is registered, which is well synchronized.
> >
> > Fixes: 3e256b8f8dfa ("NFC: add nfc subsystem core")
> > Signed-off-by: Duoming Zhou <[email protected]>
> > ---
> > Changes in v5:
> > - Replace device_is_registered() to bool variable.
> >
> > 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 5dee575fbe8..7bb4ccb1830 100644
> > --- a/include/net/nfc/nfc.h
> > +++ b/include/net/nfc/nfc.h
> > @@ -167,6 +167,7 @@ struct nfc_dev {
> > int n_targets;
> > int targets_generation;
> > struct device dev;
> > + bool dev_register;
> > bool dev_up;
> > bool fw_download_in_progress;
> > u8 rf_mode;
> > diff --git a/net/nfc/core.c b/net/nfc/core.c
> > index dc7a2404efd..52147da2286 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;
> > }
> > @@ -142,7 +142,7 @@ int nfc_dev_down(struct nfc_dev *dev)
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > goto error;
> > }
> > @@ -207,7 +207,7 @@ int nfc_start_poll(struct nfc_dev *dev, u32 im_protocols, u32 tm_protocols)
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > goto error;
> > }
> > @@ -246,7 +246,7 @@ int nfc_stop_poll(struct nfc_dev *dev)
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > goto error;
> > }
> > @@ -291,7 +291,7 @@ int nfc_dep_link_up(struct nfc_dev *dev, int target_index, u8 comm_mode)
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > goto error;
> > }
> > @@ -335,7 +335,7 @@ int nfc_dep_link_down(struct nfc_dev *dev)
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > goto error;
> > }
> > @@ -401,7 +401,7 @@ int nfc_activate_target(struct nfc_dev *dev, u32 target_idx, u32 protocol)
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > goto error;
> > }
> > @@ -448,7 +448,7 @@ int nfc_deactivate_target(struct nfc_dev *dev, u32 target_idx, u8 mode)
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > goto error;
> > }
> > @@ -495,7 +495,7 @@ int nfc_data_exchange(struct nfc_dev *dev, u32 target_idx, struct sk_buff *skb,
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > kfree_skb(skb);
> > goto error;
> > @@ -552,7 +552,7 @@ int nfc_enable_se(struct nfc_dev *dev, u32 se_idx)
> >
> > device_lock(&dev->dev);
> >
> > - if (!device_is_registered(&dev->dev)) {
> > + if (!dev->dev_register) {
> > rc = -ENODEV;
> > goto error;
> > }
> > @@ -601,7 +601,7 @@ int nfc_disable_se(struct nfc_dev *dev, u32 se_idx)
> >
> > 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);
> > @@ -1166,6 +1167,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
> > rfkill_unregister(dev->rfkill);
> > rfkill_destroy(dev->rfkill);
> > }
> > + dev->dev_register = false;
>
> We already have flag for it - dev->shutting_down. Currently it is used
> only in if device implements check_presence but I think it can be easily
> moved to common path.
>
> Having multiple fields for similar, but slightly different cases, is
> getting us closer and closer to spaghetti code.
Thanks a lot for your suggestion, I will move dev->shutting_down to
common path.
Best regards,
Duoming Zhou