lkml.org 
[lkml]   [2024]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2024-05-27 18:08    [W:6.853 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site