| 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: <20221116123435.5o76prb33mpte5hf@wse-c0155>
Date: Wed, 16 Nov 2022 13:34:35 +0100
From: Casper Andersson <casper.casan@...il.com>
To: Horatiu Vultur <horatiu.vultur@...rochip.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
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.
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);
...
> > 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
Powered by blists - more mailing lists