lkml.org 
[lkml]   [2021]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.14 725/849] net: dsa: felix: fix broken VLAN-tagged PTP under VLAN-aware bridge
    Date
    From: Vladimir Oltean <vladimir.oltean@nxp.com>

    [ Upstream commit 92f62485b3715882cd397b0cbd80a96d179b86d6 ]

    Normally it is expected that the dsa_device_ops :: rcv() method finishes
    parsing the DSA tag and consumes it, then never looks at it again.

    But commit c0bcf537667c ("net: dsa: ocelot: add hardware timestamping
    support for Felix") added support for RX timestamping in a very
    unconventional way. On this switch, a partial timestamp is available in
    the DSA header, but the driver got away with not parsing that timestamp
    right away, but instead delayed that parsing for a little longer:

    dsa_switch_rcv():
    nskb = cpu_dp->rcv(skb, dev); <------------- not here
    -> ocelot_rcv()
    ...

    skb = nskb;
    skb_push(skb, ETH_HLEN);
    skb->pkt_type = PACKET_HOST;
    skb->protocol = eth_type_trans(skb, skb->dev);

    ...

    if (dsa_skb_defer_rx_timestamp(p, skb)) <--- but here
    -> felix_rxtstamp()
    return 0;

    When in felix_rxtstamp(), this driver accounted for the fact that
    eth_type_trans() happened in the meanwhile, so it got a hold of the
    extraction header again by subtracting (ETH_HLEN + OCELOT_TAG_LEN) bytes
    from the current skb->data.

    This worked for quite some time but was quite fragile from the very
    beginning. Not to mention that having DSA tag parsing split in two
    different files, under different folders (net/dsa/tag_ocelot.c vs
    drivers/net/dsa/ocelot/felix.c) made it quite non-obvious for patches to
    come that they might break this.

    Finally, the blamed commit does the following: at the end of
    ocelot_rcv(), it checks whether the skb payload contains a VLAN header.
    If it does, and this port is under a VLAN-aware bridge, that VLAN ID
    might not be correct in the sense that the packet might have suffered
    VLAN rewriting due to TCAM rules (VCAP IS1). So we consume the VLAN ID
    from the skb payload using __skb_vlan_pop(), and take the classified
    VLAN ID from the DSA tag, and construct a hwaccel VLAN tag with the
    classified VLAN, and the skb payload is VLAN-untagged.

    The big problem is that __skb_vlan_pop() does:

    memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
    __skb_pull(skb, VLAN_HLEN);

    aka it moves the Ethernet header 4 bytes to the right, and pulls 4 bytes
    from the skb headroom (effectively also moving skb->data, by definition).
    So for felix_rxtstamp()'s fragile logic, all bets are off now.
    Instead of having the "extraction" pointer point to the DSA header,
    it actually points to 4 bytes _inside_ the extraction header.
    Corollary, the last 4 bytes of the "extraction" header are in fact 4
    stale bytes of the destination MAC address from the Ethernet header,
    from prior to the __skb_vlan_pop() movement.

    So of course, RX timestamps are completely bogus when the system is
    configured in this way.

    The fix is actually very simple: just don't structure the code like that.
    For better or worse, the DSA PTP timestamping API does not offer a
    straightforward way for drivers to present their RX timestamps, but
    other drivers (sja1105) have established a simple mechanism to carry
    their RX timestamp from dsa_device_ops :: rcv() all the way to
    dsa_switch_ops :: port_rxtstamp() and even later. That mechanism is to
    simply save the partial timestamp to the skb->cb, and complete it later.

    Question: why don't we simply populate the skb's struct
    skb_shared_hwtstamps from ocelot_rcv(), and bother with this
    complication of propagating the timestamp to felix_rxtstamp()?

    Answer: dsa_switch_ops :: port_rxtstamp() answers the question whether
    PTP packets need sleepable context to retrieve the full RX timestamp.
    Currently felix_rxtstamp() answers "no, thanks" to that question, and
    calls ocelot_ptp_gettime64() from softirq atomic context. This is
    understandable, since Felix VSC9959 is a PCIe memory-mapped switch, so
    hardware access does not require sleeping. But the felix driver is
    preparing for the introduction of other switches where hardware access
    is over a slow bus like SPI or MDIO:
    https://lore.kernel.org/lkml/20210814025003.2449143-1-colin.foster@in-advantage.com/

    So I would like to keep this code structure, so the rework needed when
    that driver will need PTP support will be minimal (answer "yes, I need
    deferred context for this skb's RX timestamp", then the partial
    timestamp will still be found in the skb->cb.

    Fixes: ea440cd2d9b2 ("net: dsa: tag_ocelot: use VLAN information from tagging header when available")
    Reported-by: Po Liu <po.liu@nxp.com>
    Cc: Yangbo Lu <yangbo.lu@nxp.com>
    Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    drivers/net/dsa/ocelot/felix.c | 9 +++------
    include/linux/dsa/ocelot.h | 1 +
    net/dsa/tag_ocelot.c | 3 +++
    3 files changed, 7 insertions(+), 6 deletions(-)

    diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
    index 0ba3762d5c219..0b81b5eb36bd1 100644
    --- a/drivers/net/dsa/ocelot/felix.c
    +++ b/drivers/net/dsa/ocelot/felix.c
    @@ -1367,12 +1367,12 @@ out:
    static bool felix_rxtstamp(struct dsa_switch *ds, int port,
    struct sk_buff *skb, unsigned int type)
    {
    - u8 *extraction = skb->data - ETH_HLEN - OCELOT_TAG_LEN;
    + u32 tstamp_lo = OCELOT_SKB_CB(skb)->tstamp_lo;
    struct skb_shared_hwtstamps *shhwtstamps;
    struct ocelot *ocelot = ds->priv;
    - u32 tstamp_lo, tstamp_hi;
    struct timespec64 ts;
    - u64 tstamp, val;
    + u32 tstamp_hi;
    + u64 tstamp;

    /* If the "no XTR IRQ" workaround is in use, tell DSA to defer this skb
    * for RX timestamping. Then free it, and poll for its copy through
    @@ -1387,9 +1387,6 @@ static bool felix_rxtstamp(struct dsa_switch *ds, int port,
    ocelot_ptp_gettime64(&ocelot->ptp_info, &ts);
    tstamp = ktime_set(ts.tv_sec, ts.tv_nsec);

    - ocelot_xfh_get_rew_val(extraction, &val);
    - tstamp_lo = (u32)val;
    -
    tstamp_hi = tstamp >> 32;
    if ((tstamp & 0xffffffff) < tstamp_lo)
    tstamp_hi--;
    diff --git a/include/linux/dsa/ocelot.h b/include/linux/dsa/ocelot.h
    index 5a6a6fad62ccc..f7e9f7d48b70b 100644
    --- a/include/linux/dsa/ocelot.h
    +++ b/include/linux/dsa/ocelot.h
    @@ -11,6 +11,7 @@
    struct ocelot_skb_cb {
    struct sk_buff *clone;
    unsigned int ptp_class; /* valid only for clones */
    + u32 tstamp_lo;
    u8 ptp_cmd;
    u8 ts_id;
    };
    diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
    index fdaae23890047..028c1350ce530 100644
    --- a/net/dsa/tag_ocelot.c
    +++ b/net/dsa/tag_ocelot.c
    @@ -63,6 +63,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
    struct dsa_port *dp;
    u8 *extraction;
    u16 vlan_tpid;
    + u64 rew_val;

    /* Revert skb->data by the amount consumed by the DSA master,
    * so it points to the beginning of the frame.
    @@ -92,6 +93,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,
    ocelot_xfh_get_qos_class(extraction, &qos_class);
    ocelot_xfh_get_tag_type(extraction, &tag_type);
    ocelot_xfh_get_vlan_tci(extraction, &vlan_tci);
    + ocelot_xfh_get_rew_val(extraction, &rew_val);

    skb->dev = dsa_master_find_slave(netdev, 0, src_port);
    if (!skb->dev)
    @@ -105,6 +107,7 @@ static struct sk_buff *ocelot_rcv(struct sk_buff *skb,

    skb->offload_fwd_mark = 1;
    skb->priority = qos_class;
    + OCELOT_SKB_CB(skb)->tstamp_lo = rew_val;

    /* Ocelot switches copy frames unmodified to the CPU. However, it is
    * possible for the user to request a VLAN modification through
    --
    2.33.0


    \
     
     \ /
      Last update: 2021-11-16 00:19    [W:4.030 / U:0.916 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site