RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Protect skb pointer used by two different kernel instances

[Selvamani Rajagopal]

> -----Original Message-----
> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> 
> 
> [Severity: Medium]
> [Finding: main-1-1]
> [Sources: opus-5]
> Should the changelog mention the other behavioural changes that come with
> this fix? The message only describes adding mutual exclusion for
> waiting_tx_skb, but the patch also:
> 
> The exact interleaving that leaks the skb is also not spelled out. Since
> this carries a Fixes: tag and will be picked up for stable, could the
> message describe the leaking sequence and the return-code change? The
> disable_traffic locking is only mentioned in the v3 changelog below the
> cut, which is dropped when the patch is applied.

Updated commit message with more info.

> 
> In oa_tc6_exit() the store happens under the lock and disable_irq() only
> runs afterwards:
> 
> spin_lock_bh(&tc6->tx_skb_lock);
> tc6->disable_traffic = true;
> spin_unlock_bh(&tc6->tx_skb_lock);
> disable_irq(tc6->spi->irq);
> 
> Can a level triggered interrupt in that window make oa_tc6_macphy_isr()
> observe a stale false, set int_flag and wake the thread, which then takes
> the early return above without doing the data chunk transfer that
> deasserts INT?

Moving disable_irq call before setting the disable_traffic flag.

> 
> > spin_lock_bh(&tc6->tx_skb_lock);
> > + if (tc6->waiting_tx_skb) {
> > + netif_stop_queue(tc6->netdev);
> > + spin_unlock_bh(&tc6->tx_skb_lock);
> > + return NETDEV_TX_BUSY;
> > + }
> 
> [Severity: Low]
> [Finding: fable-5-1-5]
> [Sources: fable-5, opus-5]
> Does moving skb_linearize() in front of the waiting_tx_skb check change the
> back-pressure policy?
> 
> Previously an xmit that hit the busy case returned NETDEV_TX_BUSY without
> touching the skb. Now every xmit linearizes first, so a transient
> skb_linearize() -ENOMEM during a busy period frees the frame and returns
> NETDEV_TX_OK, where before it would have been requeued and retried.
> 
> The kernel-doc above oa_tc6_start_xmit() still says:
> 
> * Return: NETDEV_TX_OK if the transmit ethernet frame skb added in the tx_skb_q
> * otherwise returns NETDEV_TX_BUSY.
> 
> which no longer covers the drop cases. Should that be refreshed too?


Yes. Updating the comment to cover all the return conditions.

>