Messages in this thread | | | Date | Mon, 29 Apr 2024 17:41:18 +0100 | From | Simon Horman <> | Subject | Re: [PATCH net v2] nfc: nci: Fix uninit-value in nci_rx_work |
| |
On Mon, Apr 29, 2024 at 11:30:48PM +0900, Ryosuke Yasuoka wrote: > On Sun, Apr 28, 2024 at 02:45:25PM +0100, Simon Horman wrote: > > On Sat, Apr 27, 2024 at 07:35:54PM +0900, Ryosuke Yasuoka wrote:
..
> Thank you for your comment, Simon. > > Yes, if it handles packets correctly in nci_{rsp,ntf,rx_data}_packet(), > it should not reach invalid_pkt_free and it should continue to work in > the for statement. Sorry, it is my mistake and need to fix it. > > BTW, in the current implementation, if the payload is zero, it will free > the skb and exit the for statement. I wonder it is intended. > > > > - if (!nci_plen(skb->data)) { > > > - kfree_skb(skb); > > > - break; > > > - } > > When the packet is invalid, it should be discarded but it should not > exit the for statement by break. Instead, the skb should just free and > it should handle the subsequent packet by continue. If yes, then it > may be like below, > > for (; (skb = skb_dequeue(&ndev->rx_q)); kcov_remote_stop()) { > kcov_remote_start_common(skb_get_kcov_handle(skb)); > > /* Send copy to sniffer */ > nfc_send_to_raw_sock(ndev->nfc_dev, skb, > RAW_PAYLOAD_NCI, NFC_DIRECTION_RX); > > if (!skb->len) > goto invalid_pkt_free; > > /* Process frame */ > switch (nci_mt(skb->data)) { > case NCI_MT_RSP_PKT: > if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE)) > goto invalid_pkt_free; > nci_rsp_packet(ndev, skb); > continue; <<<--- > > case NCI_MT_NTF_PKT: > if (!nci_valid_size(skb, NCI_CTRL_HDR_SIZE)) > goto invalid_pkt_free; > nci_ntf_packet(ndev, skb); > continue; <<<--- > > case NCI_MT_DATA_PKT: > if (!nci_valid_size(skb, NCI_DATA_HDR_SIZE)) > goto invalid_pkt_free; > nci_rx_data_packet(ndev, skb); > continue; <<<--- > > default: > pr_err("unknown MT 0x%x\n", nci_mt(skb->data)); > goto invalid_pkt_free; > } > invalid_pkt_free: > kfree_skb(skb); > } > > Could I hear your opinion?
Hi Yasuoka-san,
Thanks for pointing this out.
I agree that it is not good to 'break' after kfree_skb() for two reasons:
1. As you mention, the loop should keep going and process other skbs 2. kcov_remote_stop() needs to be called for each skb
I might have used a 'continue' above the invalid_pkt_free label. But I think your suggestion - using 'continue' inside the switch statement - is also correct, and seems fine to me.
Please post a v3 when you are ready.
-- pw-bot: changes-requested
| |