lkml.org 
[lkml]   [2020]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.14 04/48] gianfar: Replace skb_realloc_headroom with skb_cow_head for PTP
    Date
    From: Claudiu Manoil <claudiu.manoil@nxp.com>

    [ Upstream commit d145c9031325fed963a887851d9fa42516efd52b ]

    When PTP timestamping is enabled on Tx, the controller
    inserts the Tx timestamp at the beginning of the frame
    buffer, between SFD and the L2 frame header. This means
    that the skb provided by the stack is required to have
    enough headroom otherwise a new skb needs to be created
    by the driver to accommodate the timestamp inserted by h/w.
    Up until now the driver was relying on skb_realloc_headroom()
    to create new skbs to accommodate PTP frames. Turns out that
    this method is not reliable in this context at least, as
    skb_realloc_headroom() for PTP frames can cause random crashes,
    mostly in subsequent skb_*() calls, when multiple concurrent
    TCP streams are run at the same time with the PTP flow
    on the same device (as seen in James' report). I also noticed
    that when the system is loaded by sending multiple TCP streams,
    the driver receives cloned skbs in large numbers.
    skb_cow_head() instead proves to be stable in this scenario,
    and not only handles cloned skbs too but it's also more efficient
    and widely used in other drivers.
    The commit introducing skb_realloc_headroom in the driver
    goes back to 2009, commit 93c1285c5d92
    ("gianfar: reallocate skb when headroom is not enough for fcb").
    For practical purposes I'm referencing a newer commit (from 2012)
    that brings the code to its current structure (and fixes the PTP
    case).

    Fixes: 9c4886e5e63b ("gianfar: Fix invalid TX frames returned on error queue when time stamping")
    Reported-by: James Jurack <james.jurack@ametek.com>
    Suggested-by: Jakub Kicinski <kuba@kernel.org>
    Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
    Link: https://lore.kernel.org/r/20201029081057.8506-1-claudiu.manoil@nxp.com
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    drivers/net/ethernet/freescale/gianfar.c | 12 ++----------
    1 file changed, 2 insertions(+), 10 deletions(-)

    --- a/drivers/net/ethernet/freescale/gianfar.c
    +++ b/drivers/net/ethernet/freescale/gianfar.c
    @@ -2370,20 +2370,12 @@ static netdev_tx_t gfar_start_xmit(struc
    fcb_len = GMAC_FCB_LEN + GMAC_TXPAL_LEN;

    /* make space for additional header when fcb is needed */
    - if (fcb_len && unlikely(skb_headroom(skb) < fcb_len)) {
    - struct sk_buff *skb_new;
    -
    - skb_new = skb_realloc_headroom(skb, fcb_len);
    - if (!skb_new) {
    + if (fcb_len) {
    + if (unlikely(skb_cow_head(skb, fcb_len))) {
    dev->stats.tx_errors++;
    dev_kfree_skb_any(skb);
    return NETDEV_TX_OK;
    }
    -
    - if (skb->sk)
    - skb_set_owner_w(skb_new, skb->sk);
    - dev_consume_skb_any(skb);
    - skb = skb_new;
    }

    /* total number of fragments in the SKB */

    \
     
     \ /
      Last update: 2020-11-09 14:34    [W:2.731 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site