[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <454a29ba.1b9b1.180d576985b.Coremail.duoming@zju.edu.cn>
Date: Wed, 18 May 2022 12:39:20 +0800 (GMT+08:00)
From: duoming@....edu.cn
To: "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org>
Cc: linux-kernel@...r.kernel.org, kuba@...nel.org, davem@...emloft.net,
edumazet@...gle.com, pabeni@...hat.com, gregkh@...uxfoundation.org,
alexander.deucher@....com, broonie@...nel.org,
netdev@...r.kernel.org
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@....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
Powered by blists - more mailing lists