Messages in this thread Patch in this message | | | Date | Wed, 18 May 2022 12:39:20 +0800 (GMT+08:00) | From | duoming@zju ... | Subject | Re: Re: [PATCH net v2] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx |
| |
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@zju.edu.cn>
> > 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
| |