| Message ID | 20200316005630.9817-2-luobin9@huawei.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | hinic: BugFixes | expand |
On Mon, 16 Mar 2020 00:56:25 +0000 Luo bin wrote: > -#define MIN_SKB_LEN 17 > +#define MIN_SKB_LEN 17 > +#define HINIC_GSO_MAX_SIZE 65536 > + if (unlikely(skb->len > HINIC_GSO_MAX_SIZE && nr_sges == 1)) { > + txq->txq_stats.frag_len_overflow++; > + goto skb_error; > + } I don't think drivers should have to check this condition. We have netdev->gso_max_size which should be initialized to include/linux/netdevice.h:#define GSO_MAX_SIZE 65536 in net/core/dev.c: dev->gso_max_size = GSO_MAX_SIZE; Please send a patch to pktgen to uphold the normal stack guarantees.
From: Jakub Kicinski <kuba@kernel.org> Date: Mon, 16 Mar 2020 14:44:08 -0700 > On Mon, 16 Mar 2020 00:56:25 +0000 Luo bin wrote: >> -#define MIN_SKB_LEN 17 >> +#define MIN_SKB_LEN 17 >> +#define HINIC_GSO_MAX_SIZE 65536 > >> + if (unlikely(skb->len > HINIC_GSO_MAX_SIZE && nr_sges == 1)) { >> + txq->txq_stats.frag_len_overflow++; >> + goto skb_error; >> + } > > I don't think drivers should have to check this condition. > > We have netdev->gso_max_size which should be initialized to > > include/linux/netdevice.h:#define GSO_MAX_SIZE 65536 > > in > > net/core/dev.c: dev->gso_max_size = GSO_MAX_SIZE; > > Please send a patch to pktgen to uphold the normal stack guarantees. Agreed, the driver should not have to validate this.
Ok，I will undo this patch. On 2020/3/17 8:33, David Miller wrote: > From: Jakub Kicinski <kuba@kernel.org> > Date: Mon, 16 Mar 2020 14:44:08 -0700 > >> On Mon, 16 Mar 2020 00:56:25 +0000 Luo bin wrote: >>> -#define MIN_SKB_LEN 17 >>> +#define MIN_SKB_LEN 17 >>> +#define HINIC_GSO_MAX_SIZE 65536 >>> + if (unlikely(skb->len > HINIC_GSO_MAX_SIZE && nr_sges == 1)) { >>> + txq->txq_stats.frag_len_overflow++; >>> + goto skb_error; >>> + } >> I don't think drivers should have to check this condition. >> >> We have netdev->gso_max_size which should be initialized to >> >> include/linux/netdevice.h:#define GSO_MAX_SIZE 65536 >> >> in >> >> net/core/dev.c: dev->gso_max_size = GSO_MAX_SIZE; >> >> Please send a patch to pktgen to uphold the normal stack guarantees. > Agreed, the driver should not have to validate this. > .
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c b/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c index 966aea949c0b..dac157bc06a7 100644 --- a/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c +++ b/drivers/net/ethernet/huawei/hinic/hinic_ethtool.c @@ -582,6 +582,7 @@ static struct hinic_stats hinic_tx_queue_stats[] = { HINIC_TXQ_STAT(tx_wake), HINIC_TXQ_STAT(tx_dropped), HINIC_TXQ_STAT(big_frags_pkts), + HINIC_TXQ_STAT(frag_len_overflow), }; #define HINIC_RXQ_STAT(_stat_item) { \ diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c b/drivers/net/ethernet/huawei/hinic/hinic_main.c index 13560975c103..a9bee70bd6c7 100644 --- a/drivers/net/ethernet/huawei/hinic/hinic_main.c +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c @@ -107,6 +107,7 @@ static void update_tx_stats(struct hinic_dev *nic_dev, struct hinic_txq *txq) nic_tx_stats->tx_wake += tx_stats.tx_wake; nic_tx_stats->tx_dropped += tx_stats.tx_dropped; nic_tx_stats->big_frags_pkts += tx_stats.big_frags_pkts; + nic_tx_stats->frag_len_overflow += tx_stats.frag_len_overflow; u64_stats_update_end(&nic_tx_stats->syncp); hinic_txq_clean_stats(txq); diff --git a/drivers/net/ethernet/huawei/hinic/hinic_tx.c b/drivers/net/ethernet/huawei/hinic/hinic_tx.c index 0e13d1c7e474..3c6762086fff 100644 --- a/drivers/net/ethernet/huawei/hinic/hinic_tx.c +++ b/drivers/net/ethernet/huawei/hinic/hinic_tx.c @@ -45,9 +45,10 @@ #define HW_CONS_IDX(sq) be16_to_cpu(*(u16 *)((sq)->hw_ci_addr)) -#define MIN_SKB_LEN 17 +#define MIN_SKB_LEN 17 +#define HINIC_GSO_MAX_SIZE 65536 -#define MAX_PAYLOAD_OFFSET 221 +#define MAX_PAYLOAD_OFFSET 221 #define TRANSPORT_OFFSET(l4_hdr, skb) ((u32)((l4_hdr) - (skb)->data)) union hinic_l3 { @@ -84,6 +85,7 @@ void hinic_txq_clean_stats(struct hinic_txq *txq) txq_stats->tx_wake = 0; txq_stats->tx_dropped = 0; txq_stats->big_frags_pkts = 0; + txq_stats->frag_len_overflow = 0; u64_stats_update_end(&txq_stats->syncp); } @@ -106,6 +108,7 @@ void hinic_txq_get_stats(struct hinic_txq *txq, struct hinic_txq_stats *stats) stats->tx_wake = txq_stats->tx_wake; stats->tx_dropped = txq_stats->tx_dropped; stats->big_frags_pkts = txq_stats->big_frags_pkts; + stats->frag_len_overflow = txq_stats->frag_len_overflow; } while (u64_stats_fetch_retry(&txq_stats->syncp, start)); u64_stats_update_end(&stats->syncp); } @@ -440,7 +443,6 @@ static int hinic_tx_offload(struct sk_buff *skb, struct hinic_sq_task *task, vlan_tag >> VLAN_PRIO_SHIFT); offload |= TX_OFFLOAD_VLAN; } - if (offload) hinic_task_set_l2hdr(task, skb_network_offset(skb)); @@ -488,11 +490,14 @@ netdev_tx_t hinic_xmit_frame(struct sk_buff *skb, struct net_device *netdev) txq->txq_stats.big_frags_pkts++; u64_stats_update_end(&txq->txq_stats.syncp); } - if (nr_sges > txq->max_sges) { netdev_err(netdev, "Too many Tx sges\n"); goto skb_error; } + if (unlikely(skb->len > HINIC_GSO_MAX_SIZE && nr_sges == 1)) { + txq->txq_stats.frag_len_overflow++; + goto skb_error; + } err = tx_map_skb(nic_dev, skb, txq->sges); if (err) diff --git a/drivers/net/ethernet/huawei/hinic/hinic_tx.h b/drivers/net/ethernet/huawei/hinic/hinic_tx.h index f158b7db7fb8..ac65b4301c09 100644 --- a/drivers/net/ethernet/huawei/hinic/hinic_tx.h +++ b/drivers/net/ethernet/huawei/hinic/hinic_tx.h @@ -22,7 +22,7 @@ struct hinic_txq_stats { u64 tx_wake; u64 tx_dropped; u64 big_frags_pkts; - + u64 frag_len_overflow; struct u64_stats_sync syncp; };
some tool such as pktgen can build an illegal skb with long length but no fragments, which is unsupported for hw, so drop it Signed-off-by: Luo bin <luobin9@huawei.com> --- drivers/net/ethernet/huawei/hinic/hinic_ethtool.c | 1 + drivers/net/ethernet/huawei/hinic/hinic_main.c | 1 + drivers/net/ethernet/huawei/hinic/hinic_tx.c | 13 +++++++++---- drivers/net/ethernet/huawei/hinic/hinic_tx.h | 2 +- 4 files changed, 12 insertions(+), 5 deletions(-)