| Message ID | 20220516021028.54063-1-duoming@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] NFC: hci: fix sleep in atomic context bugs in nfc_hci_hcp_message_tx | expand |
On 16/05/2022 04:10, Duoming Zhou 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 is 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 > > This patch changes allocation mode of kzalloc and alloc_skb from > GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from > sleeping. The GFP_ATOMIC flag makes memory allocation operation > could be used in atomic context. > > Fixes: 8b8d2e08bf0d ("NFC: HCI support") > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > --- > net/nfc/hci/hcp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c > index 05c60988f59..1caf9c2086f 100644 > --- a/net/nfc/hci/hcp.c > +++ b/net/nfc/hci/hcp.c > @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe, > int hci_len, err; > bool firstfrag = true; > > - cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL); > + cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC); No, this does not look correct. This function can sleep, so it can use GFP_KERNEL. Please just look at the function before replacing any flags... Best regards, Krzysztof
Hello, On Mon, 16 May 2022 08:44:39 +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 is 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 > > > > This patch changes allocation mode of kzalloc and alloc_skb from > > GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from > > sleeping. The GFP_ATOMIC flag makes memory allocation operation > > could be used in atomic context. > > > > Fixes: 8b8d2e08bf0d ("NFC: HCI support") > > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > > --- > > net/nfc/hci/hcp.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c > > index 05c60988f59..1caf9c2086f 100644 > > --- a/net/nfc/hci/hcp.c > > +++ b/net/nfc/hci/hcp.c > > @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe, > > int hci_len, err; > > bool firstfrag = true; > > > > - cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL); > > + cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC); > > No, this does not look correct. This function can sleep, so it can use > GFP_KERNEL. Please just look at the function before replacing any flags... I am sorry, I don`t understand why nfc_hci_hcp_message_tx() can sleep. I think st21nfca_se_wt_timeout() is a timer handler, which is in interrupt context. The call tree shows the execution paths that could lead to bugs: st21nfca_hci_se_io() mod_timer(&info->se_info.bwi_timer,...) st21nfca_se_wt_timeout() //timer handler, interrupt context nfc_hci_send_event() nfc_hci_hcp_message_tx() kzalloc(..., GFP_KERNEL) //may sleep alloc_skb(..., GFP_KERNEL) //may sleep What`s more, I think the "mutex_lock(&hdev->msg_tx_mutex)" called by nfc_hci_hcp_message_tx() is also improper. Please correct me, If you think I am wrong. Thanks for your time. Best regards, Duoming Zhou
On 16/05/2022 12:18, duoming@zju.edu.cn wrote: > Hello, > > On Mon, 16 May 2022 08:44:39 +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 is 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 >>> >>> This patch changes allocation mode of kzalloc and alloc_skb from >>> GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from >>> sleeping. The GFP_ATOMIC flag makes memory allocation operation >>> could be used in atomic context. >>> >>> Fixes: 8b8d2e08bf0d ("NFC: HCI support") >>> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> >>> --- >>> net/nfc/hci/hcp.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c >>> index 05c60988f59..1caf9c2086f 100644 >>> --- a/net/nfc/hci/hcp.c >>> +++ b/net/nfc/hci/hcp.c >>> @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe, >>> int hci_len, err; >>> bool firstfrag = true; >>> >>> - cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL); >>> + cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC); >> >> No, this does not look correct. This function can sleep, so it can use >> GFP_KERNEL. Please just look at the function before replacing any flags... > > I am sorry, I don`t understand why nfc_hci_hcp_message_tx() can sleep. Why? because in line 93 it uses a mutex (which is a sleeping primitve). > > I think st21nfca_se_wt_timeout() is a timer handler, which is in interrupt context. > The call tree shows the execution paths that could lead to bugs: > > st21nfca_hci_se_io() > mod_timer(&info->se_info.bwi_timer,...) > st21nfca_se_wt_timeout() //timer handler, interrupt context > nfc_hci_send_event() > nfc_hci_hcp_message_tx() > kzalloc(..., GFP_KERNEL) //may sleep > alloc_skb(..., GFP_KERNEL) //may sleep > > What`s more, I think the "mutex_lock(&hdev->msg_tx_mutex)" called by nfc_hci_hcp_message_tx() > is also improper. > > Please correct me, If you think I am wrong. Thanks for your time. Your patch is not correct in current semantics of this function. This function can sleep (because it uses a mutex), so the mistake is rather calling it from interrupt context. Best regards, Krzysztof
Hello, On Mon, 16 May 2022 12:43:07 +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 is 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 > >>> > >>> This patch changes allocation mode of kzalloc and alloc_skb from > >>> GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from > >>> sleeping. The GFP_ATOMIC flag makes memory allocation operation > >>> could be used in atomic context. > >>> > >>> Fixes: 8b8d2e08bf0d ("NFC: HCI support") > >>> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > >>> --- > >>> net/nfc/hci/hcp.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c > >>> index 05c60988f59..1caf9c2086f 100644 > >>> --- a/net/nfc/hci/hcp.c > >>> +++ b/net/nfc/hci/hcp.c > >>> @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe, > >>> int hci_len, err; > >>> bool firstfrag = true; > >>> > >>> - cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL); > >>> + cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC); > >> > >> No, this does not look correct. This function can sleep, so it can use > >> GFP_KERNEL. Please just look at the function before replacing any flags... > > > > I am sorry, I don`t understand why nfc_hci_hcp_message_tx() can sleep. > > Why? because in line 93 it uses a mutex (which is a sleeping primitve). > > > > > I think st21nfca_se_wt_timeout() is a timer handler, which is in interrupt context. > > The call tree shows the execution paths that could lead to bugs: > > > > st21nfca_hci_se_io() > > mod_timer(&info->se_info.bwi_timer,...) > > st21nfca_se_wt_timeout() //timer handler, interrupt context > > nfc_hci_send_event() > > nfc_hci_hcp_message_tx() > > kzalloc(..., GFP_KERNEL) //may sleep > > alloc_skb(..., GFP_KERNEL) //may sleep > > > > What`s more, I think the "mutex_lock(&hdev->msg_tx_mutex)" called by nfc_hci_hcp_message_tx() > > is also improper. > > > > Please correct me, If you think I am wrong. Thanks for your time. > > Your patch is not correct in current semantics of this function. This > function can sleep (because it uses a mutex), so the mistake is rather > calling it from interrupt context. We have to call nfc_hci_send_event() in st21nfca_se_wt_timeout(), because we need to send a reset request as recovery procedure. I think replace GFP_KERNEL to GFP_ATOMIC and replace mutex_lock to spin_lock in nfc_hci_hcp_message_tx() is better. What's more, in order to synchronize with other functions related with hci message TX, We need to replace the mutex_lock(&hdev->msg_tx_mutex) to spin_lock in other functions as well. I sent "patch v2" just now. Best regards, Duoming Zhou
On 17/05/2022 12:56, duoming@zju.edu.cn wrote: > Hello, > > On Mon, 16 May 2022 12:43:07 +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 is 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 >>>>> >>>>> This patch changes allocation mode of kzalloc and alloc_skb from >>>>> GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from >>>>> sleeping. The GFP_ATOMIC flag makes memory allocation operation >>>>> could be used in atomic context. >>>>> >>>>> Fixes: 8b8d2e08bf0d ("NFC: HCI support") >>>>> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> >>>>> --- >>>>> net/nfc/hci/hcp.c | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c >>>>> index 05c60988f59..1caf9c2086f 100644 >>>>> --- a/net/nfc/hci/hcp.c >>>>> +++ b/net/nfc/hci/hcp.c >>>>> @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe, >>>>> int hci_len, err; >>>>> bool firstfrag = true; >>>>> >>>>> - cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL); >>>>> + cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC); >>>> >>>> No, this does not look correct. This function can sleep, so it can use >>>> GFP_KERNEL. Please just look at the function before replacing any flags... >>> >>> I am sorry, I don`t understand why nfc_hci_hcp_message_tx() can sleep. >> >> Why? because in line 93 it uses a mutex (which is a sleeping primitve). >> >>> >>> I think st21nfca_se_wt_timeout() is a timer handler, which is in interrupt context. >>> The call tree shows the execution paths that could lead to bugs: >>> >>> st21nfca_hci_se_io() >>> mod_timer(&info->se_info.bwi_timer,...) >>> st21nfca_se_wt_timeout() //timer handler, interrupt context >>> nfc_hci_send_event() >>> nfc_hci_hcp_message_tx() >>> kzalloc(..., GFP_KERNEL) //may sleep >>> alloc_skb(..., GFP_KERNEL) //may sleep >>> >>> What`s more, I think the "mutex_lock(&hdev->msg_tx_mutex)" called by nfc_hci_hcp_message_tx() >>> is also improper. >>> >>> Please correct me, If you think I am wrong. Thanks for your time. >> >> Your patch is not correct in current semantics of this function. This >> function can sleep (because it uses a mutex), so the mistake is rather >> calling it from interrupt context. > > We have to call nfc_hci_send_event() in st21nfca_se_wt_timeout(), because we need to send > a reset request as recovery procedure. I think replace GFP_KERNEL to GFP_ATOMIC and replace > mutex_lock to spin_lock in nfc_hci_hcp_message_tx() is better. > > What's more, in order to synchronize with other functions related with hci message TX, > We need to replace the mutex_lock(&hdev->msg_tx_mutex) to spin_lock in other functions > as well. I sent "patch v2" just now. You sent v2 one minute before replying here... that's not how discussion work. Please do not sent next version before reaching some consensus. Best regards, Krzysztof
Hello, On Tue, 17 May 2022 13:39:44 +0200 wrote: > You sent v2 one minute before replying here... that's not how discussion > work. Please do not sent next version before reaching some consensus. I am sorry. Before reaching some consensus, I will not send next version in the future. Thanks for your guidance. Best regards, Duoming Zhou
diff --git a/net/nfc/hci/hcp.c b/net/nfc/hci/hcp.c index 05c60988f59..1caf9c2086f 100644 --- a/net/nfc/hci/hcp.c +++ b/net/nfc/hci/hcp.c @@ -30,7 +30,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe, int hci_len, err; bool firstfrag = true; - cmd = kzalloc(sizeof(struct hci_msg), GFP_KERNEL); + cmd = kzalloc(sizeof(*cmd), GFP_ATOMIC); if (cmd == NULL) return -ENOMEM; @@ -58,7 +58,7 @@ int nfc_hci_hcp_message_tx(struct nfc_hci_dev *hdev, u8 pipe, data_link_len + ndev->tx_tailroom; hci_len -= data_link_len; - skb = alloc_skb(skb_len, GFP_KERNEL); + skb = alloc_skb(skb_len, GFP_ATOMIC); if (skb == NULL) { err = -ENOMEM; goto out_skb_err;
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 is 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 This patch changes allocation mode of kzalloc and alloc_skb from GFP_KERNEL to GFP_ATOMIC in order to prevent atomic context from sleeping. The GFP_ATOMIC flag makes memory allocation operation could be used in atomic context. Fixes: 8b8d2e08bf0d ("NFC: HCI support") Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> --- net/nfc/hci/hcp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)