netdev - Re: net/mlx4_en: Pad ethernet packets smaller than 17 bytes

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: <20140227140203.GA13112@mtl-eit-vdi-22.mtl.labs.mlnx>
Date:	Thu, 27 Feb 2014 16:02:05 +0200
From:	Amir Vadai <amirv@...lanox.com>
To:	David Laight <David.Laight@...LAB.COM>
Cc:	"David S. Miller" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Yevgeny Petrilin <yevgenyp@...lanox.com>,
	Or Gerlitz <ogerlitz@...lanox.com>,
	Eugenia Emantayev <eugenia@...lanox.com>
Subject: Re: net/mlx4_en: Pad ethernet packets smaller than 17 bytes

On 27/02/14 13:08 +0000, David Laight wrote:
> From: Amir Vadai
> > Hardware can't accept packets smaller than 17 bytes. Therefore need to pad with
> > zeros.
> 
> Can they actually happen?
> A 16 byte packet would only have 2 bytes following the ethertype/length.
> The shortest LLC packets have 3 bytes.
Raw sockets might send such packets and it will put the HW into
unstable state.

> It may well be safe to discard them.
Yep - that another option.

> 
> ...
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> > index 8dc7637..268cc4a 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> > @@ -585,12 +585,19 @@ static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc, struct sk_buff *sk
> >  	int spc = MLX4_INLINE_ALIGN - CTRL_SIZE - sizeof *inl;
> > 
> >  	if (skb->len <= spc) {
> > -		inl->byte_count = cpu_to_be32(1 << 31 | skb->len);
> > +		inl->byte_count = cpu_to_be32(1 << 31 |
> > +					      max_t(typeof(skb->len),
> > +						    skb->len,
> > +						    MIN_PKT_LEN));
> >  		skb_copy_from_linear_data(skb, inl + 1, skb_headlen(skb));
> >  		if (skb_shinfo(skb)->nr_frags)
> >  			memcpy(((void *)(inl + 1)) + skb_headlen(skb), fragptr,
> >  			       skb_frag_size(&skb_shinfo(skb)->frags[0]));
> 
> Is there guaranteed to be only 1 fragment here?
> 
Yes, skb will not be classified as inline-able if nr_frags != 1.
See is_inline()

> > +		if (skb->len < MIN_PKT_LEN)
> > +			memset(((void *)(inl + 1)) + skb->len, 0,
> > +			       MIN_PKT_LEN - skb->len);
> > +
> 
> In any case you don't want 2 checks for skb->len < MIN_PKT_LEN,
> so reassign to inl->byte_count here.
> An unlikely() probably wouldn't go amiss either.
Right - will fix it for V1

> 
> 	David
> 

Thanks,
Amir
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html