| 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
| ||
|
Date: Thu, 17 Nov 2022 13:22:44 +0100
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Casper Andersson <casper.casan@...il.com>
CC: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Lars Povlsen <lars.povlsen@...rochip.com>,
"Steen Hegelund" <Steen.Hegelund@...rochip.com>,
Daniel Machon <daniel.machon@...rochip.com>,
<UNGLinuxDriver@...rochip.com>,
"Richard Cochran" <richardcochran@...il.com>,
<netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: microchip: sparx5: correctly free skb in xmit
The 11/16/2022 13:34, Casper Andersson wrote:
>
> Hi Horatiu,
>
> On 2022-11-16 12:41, Horatiu Vultur wrote:
> > The 11/16/2022 10:57, Casper Andersson wrote:
> >
> > Hi Casper,
> >
> > >
> > > consume_skb on transmitted, kfree_skb on dropped, do not free on TX_BUSY.
> > >
> > > Previously the xmit function could return -EBUSY without freeing, which
> > > supposedly is interpreted as a drop. And was using kfree on successfully
> > > transmitted packets.
> > > https://lore.kernel.org/netdev/20220920072948.33c25dd2@kernel.org/t/#mdb821eb507a207dd5e27683239ffa7ec7199421a
> > >
> > > Fixes: 10615907e9b5 ("net: sparx5: switchdev: adding frame DMA functionality")
> >
> > Actually the fix shouldn't be:
> > 70dfe25cd866 ("net: sparx5: Update extraction/injection for timestamping") ?
> > Because this commit introduced the function sparx5_ptp_txtstamp_request,
> > and it didn't free the skb on error path.
> >
>
> As I understood Jakub's comment (linked above) part of the issue was
> that sparx5_inject (and sparx5_ptp_txtstamp_request) returns -EBUSY
> instead of NETDEV_TX_BUSY (which then gets returned further). So maybe
> it should be separate patches for the two cases.
Ah.. I think now I understand it. It is up to you how to take this.
>
> While on the topic, I noticed another possible issue with
> sparx5_fdma_xmit where it increments dropped, but keeps going with
> transmit. I assume it should return and indicate dropped here.
>
> if (!(db_hw->status & FDMA_DCB_STATUS_DONE))
> tx->dropped++;
> db = list_first_entry(&tx->db_list, struct sparx5_db, list);
You are correct, that looks wrong also to me.
It should return and drop the frame.
> ...
>
> > > Signed-off-by: Casper Andersson <casper.casan@...il.com>
> > > ---
> > > I am not entirely sure about the following construct which is present in
> > > both sparx5 and lan966x drivers and returns before consuming the skb. Is
> > > there any reason it does not free the skb?
> >
> > Yes, this case will happen when the HW is configured to timestamp frames
> > and return the timestamp back to the SW. And in that case the frame is freed
> > once the timestamp is received. Have a look in the function
> > 'sparx5_ptp_irq_handler'
>
> Alright, that makes sense.
>
> >
> > >
> > > if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> > > SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> > > return NETDEV_TX_OK;
> > >
> > >
> > >
> > >
> > > .../ethernet/microchip/sparx5/sparx5_fdma.c | 2 +-
> > > .../ethernet/microchip/sparx5/sparx5_main.h | 2 +-
> > > .../ethernet/microchip/sparx5/sparx5_packet.c | 47 ++++++++++---------
> > > 3 files changed, 27 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > > index 66360c8c5a38..302e7ff55585 100644
> > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > > @@ -306,7 +306,7 @@ static struct sparx5_tx_dcb_hw *sparx5_fdma_next_dcb(struct sparx5_tx *tx,
> > > return next_dcb;
> > > }
> > >
> > > -int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
> > > +netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb)
> > > {
> > > struct sparx5_tx_dcb_hw *next_dcb_hw;
> > > struct sparx5_tx *tx = &sparx5->tx;
> > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > > index 7a83222caa73..34b8d11f76df 100644
> > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_main.h
> > > @@ -312,7 +312,7 @@ void sparx5_port_inj_timer_setup(struct sparx5_port *port);
> > > /* sparx5_fdma.c */
> > > int sparx5_fdma_start(struct sparx5 *sparx5);
> > > int sparx5_fdma_stop(struct sparx5 *sparx5);
> > > -int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
> > > +netdev_tx_t sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb);
> > > irqreturn_t sparx5_fdma_handler(int irq, void *args);
> > >
> > > /* sparx5_mactable.c */
> > > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > > index 83c16ca5b30f..6fc1c1e410f6 100644
> > > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_packet.c
> > > @@ -159,10 +159,10 @@ static void sparx5_xtr_grp(struct sparx5 *sparx5, u8 grp, bool byte_swap)
> > > netif_rx(skb);
> > > }
> > >
> > > -static int sparx5_inject(struct sparx5 *sparx5,
> > > - u32 *ifh,
> > > - struct sk_buff *skb,
> > > - struct net_device *ndev)
> > > +static netdev_tx_t sparx5_inject(struct sparx5 *sparx5,
> > > + u32 *ifh,
> > > + struct sk_buff *skb,
> > > + struct net_device *ndev)
> > > {
> > > int grp = INJ_QUEUE;
> > > u32 val, w, count;
> > > @@ -172,7 +172,7 @@ static int sparx5_inject(struct sparx5 *sparx5,
> > > if (!(QS_INJ_STATUS_FIFO_RDY_GET(val) & BIT(grp))) {
> > > pr_err_ratelimited("Injection: Queue not ready: 0x%lx\n",
> > > QS_INJ_STATUS_FIFO_RDY_GET(val));
> > > - return -EBUSY;
> > > + return NETDEV_TX_BUSY;
> > > }
> > >
> > > /* Indicate SOF */
> > > @@ -234,9 +234,8 @@ netdev_tx_t sparx5_port_xmit_impl(struct sk_buff *skb, struct net_device *dev)
> > > sparx5_set_port_ifh(ifh, port->portno);
> > >
> > > if (sparx5->ptp && skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> > > - ret = sparx5_ptp_txtstamp_request(port, skb);
> > > - if (ret)
> > > - return ret;
> > > + if (sparx5_ptp_txtstamp_request(port, skb))
> > > + goto drop;
> >
> > Is it not possible to return busy also here?
> > The reason why I ask this is because of the following case.
> > Imagine you need to send a frame and timestamp it, but the HW
> > currrently don't have enough slots to store the TX timestamp. In that
> > case is better to return BUSY because maybe in short time there will be
> > slots. Than just dropping the frame, because if the frame is dropped,
> > then there is no chance to get the TX timestamp, and who needs this will
> > stop working.
>
> Yes, we can do that. Thanks for clarifying, I hadn't quite understood
> the PTP parts of this.
>
> >
> > >
> > > sparx5_set_port_ifh_rew_op(ifh, SPARX5_SKB_CB(skb)->rew_op);
> > > sparx5_set_port_ifh_pdu_type(ifh, SPARX5_SKB_CB(skb)->pdu_type);
> > > @@ -250,23 +249,27 @@ netdev_tx_t sparx5_port_xmit_impl(struct sk_buff *skb, struct net_device *dev)
> > > else
> > > ret = sparx5_inject(sparx5, ifh, skb, dev);
> > >
> > > - if (ret == NETDEV_TX_OK) {
> > > - stats->tx_bytes += skb->len;
> > > - stats->tx_packets++;
> > > + if (ret == NETDEV_TX_BUSY)
> > > + goto busy;
> > >
> > > - if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> > > - SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> > > - return ret;
> > > + stats->tx_bytes += skb->len;
> > > + stats->tx_packets++;
> > >
> > > - dev_kfree_skb_any(skb);
> > > - } else {
> > > - stats->tx_dropped++;
> > > + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> > > + SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> > > + return NETDEV_TX_OK;
> > >
> > > - if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> > > - SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> > > - sparx5_ptp_txtstamp_release(port, skb);
> > > - }
> > > - return ret;
> > > + dev_consume_skb_any(skb);
> > > + return NETDEV_TX_OK;
> > > +drop:
> > > + stats->tx_dropped++;
> > > + dev_kfree_skb_any(skb);
> > > + return NETDEV_TX_OK;
> > > +busy:
> > > + if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
> > > + SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
> > > + sparx5_ptp_txtstamp_release(port, skb);
> > > + return NETDEV_TX_BUSY;
> > > }
> > >
> > > static enum hrtimer_restart sparx5_injection_timeout(struct hrtimer *tmr)
> > > --
> > > 2.34.1
> > >
> >
> > --
> > /Horatiu
--
/Horatiu
Powered by blists - more mailing lists