Re: Re: [PATCH net v2] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx

[duoming]

Hello,

On Tue, 17 May 2022 17:28:51 +0200 Krzysztof wrote:

> >>> There are sleep in atomic context bugs when the request to secure
> >>> element of st21nfca is timeout. The root cause is that kzalloc and
> >>> alloc_skb with GFP_KERNEL parameter and mutex_lock are called in
> >>> st21nfca_se_wt_timeout which is a timer handler. The call tree shows
> >>> the execution paths that could lead to bugs:
> >>>
> >>>    (Interrupt context)
> >>> st21nfca_se_wt_timeout
> >>>   nfc_hci_send_event
> >>>     nfc_hci_hcp_message_tx
> >>>       kzalloc(..., GFP_KERNEL) //may sleep
> >>>       alloc_skb(..., GFP_KERNEL) //may sleep
> >>>       mutex_lock() //may sleep
> >>>
> >>> This patch changes allocation mode of kzalloc and alloc_skb from
> >>> GFP_KERNEL to GFP_ATOMIC and changes mutex_lock to spin_lock in
> >>> order to prevent atomic context from sleeping.
> >>>
> >>> Fixes: 2130fb97fecf ("NFC: st21nfca: Adding support for secure element")
> >>> Signed-off-by: Duoming Zhou <duoming@xxxxxxxxxx>

> > The nfc_hci_hcp_message_tx() is called by both process context(hci_dev_up and so on)
> > and interrupt context(st21nfca_se_wt_timeout()). The process context(hci_dev_up and so on)
> > calls device_lock, but I think calling spin_lock() within device_lock() is ok. There is
> > no device_lock() called within spin_lock(). 
> 
> There is.
> 
> nfc_hci_failure -> spin lock -> nfc_driver_failure -> nfc_targets_found
> -> device_lock
> 
> I found it just by a very quick look, so I suspect there are several
> other places, not really checked.

I agree with you, the spin_lock is not a good solution to this problem. There is another solution:

We could put the nfc_hci_send_event() of st21nfca_se_wt_timeout() in a work item, then, using
schedule_work() in st21nfca_se_wt_timeout() to execute the work item. The schedule_work() will
wake up another kernel thread which is in process context to execute the bottom half of the interrupt, 
so it allows sleep.

The following is the details.

diff --git a/drivers/nfc/st21nfca/se.c b/drivers/nfc/st21nfca/se.c
index c922f10d0d7..1e98467dbf7 100644
--- a/drivers/nfc/st21nfca/se.c
+++ b/drivers/nfc/st21nfca/se.c
@@ -241,7 +241,7 @@ int st21nfca_hci_se_io(struct nfc_hci_dev *hdev, u32 se_idx,
 }
 EXPORT_SYMBOL(st21nfca_hci_se_io);

-static void st21nfca_se_wt_timeout(struct timer_list *t)
+static void st21nfca_se_wt_work(struct work_struct *work)
 {
        /* 
         * No answer from the secure element
@@ -254,8 +254,9 @@ static void st21nfca_se_wt_timeout(struct timer_list *t)
         */
        /* hardware reset managed through VCC_UICC_OUT power supply */
        u8 param = 0x01;
-       struct st21nfca_hci_info *info = from_timer(info, t,
-                                                   se_info.bwi_timer);
+       struct st21nfca_hci_info *info = container_of(work,
+                                               struct st21nfca_hci_info,
+                                               se_info.timeout_work);

        info->se_info.bwi_active = false;
 
@@ -271,6 +272,13 @@ static void st21nfca_se_wt_timeout(struct timer_list *t)
        info->se_info.cb(info->se_info.cb_context, NULL, 0, -ETIME);
 }

+static void st21nfca_se_wt_timeout(struct timer_list *t)
+{
+       struct st21nfca_hci_info *info = from_timer(info, t, se_info.bwi_timer);
+
+       schedule_work(&info->se_info.timeout_work);
+}
+
 static void st21nfca_se_activation_timeout(struct timer_list *t)
 {
        struct st21nfca_hci_info *info = from_timer(info, t,
@@ -389,6 +397,7 @@ void st21nfca_se_init(struct nfc_hci_dev *hdev)
        struct st21nfca_hci_info *info = nfc_hci_get_clientdata(hdev);

        init_completion(&info->se_info.req_completion);
+       INIT_WORK(&info->se_info.timeout_work, st21nfca_se_wt_work);
        /* initialize timers */
        timer_setup(&info->se_info.bwi_timer, st21nfca_se_wt_timeout, 0);
        info->se_info.bwi_active = false;
@@ -416,6 +425,7 @@ void st21nfca_se_deinit(struct nfc_hci_dev *hdev)
        if (info->se_info.se_active)
                del_timer_sync(&info->se_info.se_active_timer);

+       cancel_work_sync(&info->se_info.timeout_work);
        info->se_info.bwi_active = false;
        info->se_info.se_active = false;
 }
diff --git a/drivers/nfc/st21nfca/st21nfca.h b/drivers/nfc/st21nfca/st21nfca.h
index cb6ad916be9..ae6771cc989 100644
--- a/drivers/nfc/st21nfca/st21nfca.h
+++ b/drivers/nfc/st21nfca/st21nfca.h
@@ -141,6 +141,7 @@ struct st21nfca_se_info {

        se_io_cb_t cb;
        void *cb_context;
+       struct work_struct timeout_work;
 };

 struct st21nfca_hci_info {

Do you think this solution is better?

Best regards,
Duoming Zhou