netdev - Re: [PATCH 2/2] enetc: support PTP Sync packet one-step timestamping

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8fa3394e-847f-a3fa-438a-1b357b5726fa@gmail.com>
Date:   Sun, 28 Mar 2021 10:51:59 +0300
From:   Claudiu Manoil <claudiu.manoil@...il.com>
To:     Yangbo Lu <yangbo.lu@....com>, netdev@...r.kernel.org
Cc:     "David S . Miller" <davem@...emloft.net>,
        Richard Cochran <richardcochran@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Jakub Kicinski <kuba@...nel.org>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH 2/2] enetc: support PTP Sync packet one-step timestamping

Hi Yangbo,
Pls add the [net-next] prefix to the subject of these patches next time, 
to avoid the patchwork warnings and let reviewers know where to apply them.

On 26.03.2021 10:35, Yangbo Lu wrote:
[...]> +netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> +	u8 udp, msgtype, twostep;
> +	u16 offset1, offset2;
> +
> +	/* Mark tx timestamp type on skb->cb[0] if requires */
> +	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> +	    (priv->active_offloads & ENETC_F_TX_TSTAMP_MASK)) {
> +		skb->cb[0] = priv->active_offloads & ENETC_F_TX_TSTAMP_MASK;
> +	} else {
> +		skb->cb[0] = 0;
> +	}
> +
> +	if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
> +		/* For one-step PTP sync packet, queue it */
> +		if (!enetc_ptp_parse(skb, &udp, &msgtype, &twostep,
> +				     &offset1, &offset2)) {
> +			if (msgtype == PTP_MSGTYPE_SYNC && twostep == 0) {
> +				skb_queue_tail(&priv->tx_skbs, skb);
> +				queue_work(priv->enetc_ptp_wq,
> +					   &priv->tx_onestep_tstamp);
> +				return NETDEV_TX_OK;
> +			}
> +		}
> +
> +		/* Fall back to two-step timestamp for other packets */
> +		skb->cb[0] = ENETC_F_TX_TSTAMP;
> +	}
> +
> +	return enetc_start_xmit(skb, ndev);
> +}
> +
[...]
> +static void enetc_tx_onestep_tstamp(struct work_struct *work)
> +{
> +	struct enetc_ndev_priv *priv;
> +	struct sk_buff *skb;
> +
> +	priv = container_of(work, struct enetc_ndev_priv, tx_onestep_tstamp);
> +
> +	while (true) {
> +		skb = skb_dequeue(&priv->tx_skbs);
> +		if (!skb)
> +			return;
> +
> +		/* Lock before TX one-step timestamping packet, and release
> +		 * when the packet has been sent on hardware, or transmit
> +		 * failure.
> +		 */
> +		mutex_lock(&priv->onestep_tstamp_lock);
> +		enetc_start_xmit(skb, priv->ndev);
> +	}
> +}
> +
What happens if the work queue tries to send the ptp packet concurrently 
with a regular packet being sent by the stack, via .ndo_start_xmit?
If both skbs are targetting the same tx_ring then we have a concurrency 
problem, as enetc_map_tx_buffs(tx_ring, skb) is not thread safe!

Regards,
Claudiu