| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200923200800.t7y47fagvgffw4ya@soft-dev3.localdomain>
Date: Wed, 23 Sep 2020 22:08:00 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: <davem@...emloft.net>, <netdev@...r.kernel.org>,
<yangbo.lu@....com>, <xiaoliang.yang_1@....com>,
<UNGLinuxDriver@...rochip.com>, <claudiu.manoil@....com>,
<alexandre.belloni@...tlin.com>, <andrew@...n.ch>,
<vivien.didelot@...il.com>, <f.fainelli@...il.com>,
<kuba@...nel.org>, <richardcochran@...il.com>
Subject: Re: [PATCH net-next] net: mscc: ocelot: always pass skb clone to
ocelot_port_add_txtstamp_skb
The 09/23/2020 14:24, Vladimir Oltean wrote:
>
Hi Vladimir,
I have just one question, please see bellow.
> Currently, ocelot switchdev passes the skb directly to the function that
> enqueues it to the list of skb's awaiting a TX timestamp. Whereas the
> felix DSA driver first clones the skb, then passes the clone to this
> queue.
>
> This matters because in the case of felix, the common IRQ handler, which
> is ocelot_get_txtstamp(), currently clones the clone, and frees the
> original clone. This is useless and can be simplified by using
> skb_complete_tx_timestamp() instead of skb_tstamp_tx().
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> drivers/net/dsa/ocelot/felix.c | 5 ++++-
> drivers/net/ethernet/mscc/ocelot.c | 30 ++++++++++----------------
> drivers/net/ethernet/mscc/ocelot_net.c | 22 +++++++++++++++----
> include/soc/mscc/ocelot.h | 4 ++--
> net/dsa/tag_ocelot.c | 6 +++---
> 5 files changed, 38 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 0b2bb8d7325c..1ec4fea5a546 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -668,8 +668,11 @@ static bool felix_txtstamp(struct dsa_switch *ds, int port,
> struct ocelot *ocelot = ds->priv;
> struct ocelot_port *ocelot_port = ocelot->ports[port];
>
> - if (!ocelot_port_add_txtstamp_skb(ocelot_port, clone))
> + if (ocelot->ptp && (skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP) &&
> + ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> + ocelot_port_add_txtstamp_skb(ocelot, port, clone);
> return true;
> + }
>
> return false;
> }
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 977d2263cbe1..58b969b85643 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -413,26 +413,20 @@ void ocelot_port_disable(struct ocelot *ocelot, int port)
> }
> EXPORT_SYMBOL(ocelot_port_disable);
>
> -int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
> - struct sk_buff *skb)
> +void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
> + struct sk_buff *clone)
> {
> - struct skb_shared_info *shinfo = skb_shinfo(skb);
> - struct ocelot *ocelot = ocelot_port->ocelot;
> + struct ocelot_port *ocelot_port = ocelot->ports[port];
>
> - if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP &&
> - ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> - spin_lock(&ocelot_port->ts_id_lock);
> + spin_lock(&ocelot_port->ts_id_lock);
>
> - shinfo->tx_flags |= SKBTX_IN_PROGRESS;
> - /* Store timestamp ID in cb[0] of sk_buff */
> - skb->cb[0] = ocelot_port->ts_id;
> - ocelot_port->ts_id = (ocelot_port->ts_id + 1) % 4;
> - skb_queue_tail(&ocelot_port->tx_skbs, skb);
> + skb_shinfo(clone)->tx_flags |= SKBTX_IN_PROGRESS;
> + /* Store timestamp ID in cb[0] of sk_buff */
> + clone->cb[0] = ocelot_port->ts_id;
> + ocelot_port->ts_id = (ocelot_port->ts_id + 1) % 4;
> + skb_queue_tail(&ocelot_port->tx_skbs, clone);
>
> - spin_unlock(&ocelot_port->ts_id_lock);
> - return 0;
> - }
> - return -ENODATA;
> + spin_unlock(&ocelot_port->ts_id_lock);
> }
> EXPORT_SYMBOL(ocelot_port_add_txtstamp_skb);
>
> @@ -511,9 +505,7 @@ void ocelot_get_txtstamp(struct ocelot *ocelot)
> /* Set the timestamp into the skb */
> memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> - skb_tstamp_tx(skb_match, &shhwtstamps);
> -
> - dev_kfree_skb_any(skb_match);
> + skb_complete_tx_timestamp(skb_match, &shhwtstamps);
>
> /* Next ts */
> ocelot_write(ocelot, SYS_PTP_NXT_PTP_NXT, SYS_PTP_NXT);
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 8490e42e9e2d..028a0150f97d 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -330,7 +330,6 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
> u8 grp = 0; /* Send everything on CPU group 0 */
> unsigned int i, count, last;
> int port = priv->chip_port;
> - bool do_tstamp;
>
> val = ocelot_read(ocelot, QS_INJ_STATUS);
> if (!(val & QS_INJ_STATUS_FIFO_RDY(BIT(grp))) ||
> @@ -345,7 +344,23 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
> info.vid = skb_vlan_tag_get(skb);
>
> /* Check if timestamping is needed */
> - do_tstamp = (ocelot_port_add_txtstamp_skb(ocelot_port, skb) == 0);
> + if (ocelot->ptp && (shinfo->tx_flags & SKBTX_HW_TSTAMP)) {
> + info.rew_op = ocelot_port->ptp_cmd;
> +
> + if (ocelot_port->ptp_cmd == IFH_REW_OP_TWO_STEP_PTP) {
> + struct sk_buff *clone;
> +
> + clone = skb_clone_sk(skb);
> + if (!clone) {
> + kfree_skb(skb);
> + return NETDEV_TX_OK;
Why do you return NETDEV_TX_OK?
Because the frame is not sent yet.
> + }
> +
> + ocelot_port_add_txtstamp_skb(ocelot, port, clone);
> +
> + info.rew_op |= clone->cb[0] << 3;
> + }
> + }
>
> if (ocelot->ptp && shinfo->tx_flags & SKBTX_HW_TSTAMP) {
> info.rew_op = ocelot_port->ptp_cmd;
> @@ -383,8 +398,7 @@ static int ocelot_port_xmit(struct sk_buff *skb, struct net_device *dev)
> dev->stats.tx_packets++;
> dev->stats.tx_bytes += skb->len;
>
> - if (!do_tstamp)
> - dev_kfree_skb_any(skb);
> + kfree_skb(skb);
>
> return NETDEV_TX_OK;
> }
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index df252c22f985..2ca7ba829c76 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -711,8 +711,8 @@ int ocelot_vlan_add(struct ocelot *ocelot, int port, u16 vid, bool pvid,
> int ocelot_vlan_del(struct ocelot *ocelot, int port, u16 vid);
> int ocelot_hwstamp_get(struct ocelot *ocelot, int port, struct ifreq *ifr);
> int ocelot_hwstamp_set(struct ocelot *ocelot, int port, struct ifreq *ifr);
> -int ocelot_port_add_txtstamp_skb(struct ocelot_port *ocelot_port,
> - struct sk_buff *skb);
> +void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port,
> + struct sk_buff *clone);
> void ocelot_get_txtstamp(struct ocelot *ocelot);
> void ocelot_port_set_maxlen(struct ocelot *ocelot, int port, size_t sdu);
> int ocelot_get_max_mtu(struct ocelot *ocelot, int port);
> diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c
> index b4fc05cafaa6..d1a7e224adff 100644
> --- a/net/dsa/tag_ocelot.c
> +++ b/net/dsa/tag_ocelot.c
> @@ -137,6 +137,7 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
> struct net_device *netdev)
> {
> struct dsa_port *dp = dsa_slave_to_port(netdev);
> + struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
> struct dsa_switch *ds = dp->ds;
> struct ocelot *ocelot = ds->priv;
> struct ocelot_port *ocelot_port;
> @@ -159,9 +160,8 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb,
> qos_class = skb->priority;
> packing(injection, &qos_class, 19, 17, OCELOT_TAG_LEN, PACK, 0);
>
> - if (ocelot->ptp && (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)) {
> - struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
> -
> + /* TX timestamping was requested */
> + if (clone) {
> rew_op = ocelot_port->ptp_cmd;
> /* Retrieve timestamp ID populated inside skb->cb[0] of the
> * clone by ocelot_port_add_txtstamp_skb
> --
> 2.25.1
>
--
/Horatiu
Powered by blists - more mailing lists