[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f81f8974-3c1b-4e34-ce51-5e0e7472079b@daynix.com>
Date: Wed, 25 Jan 2023 13:04:10 +0900
From: Akihiko Odaki <akihiko.odaki@...nix.com>
To: Paul Menzel <pmenzel@...gen.mpg.de>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, netdev@...r.kernel.org,
Yan Vugenfirer <yvugenfi@...hat.com>,
linux-kernel@...r.kernel.org,
Jesse Brandeburg <jesse.brandeburg@...el.com>,
Yuri Benditovich <yuri.benditovich@...nix.com>,
Eric Dumazet <edumazet@...gle.com>,
intel-wired-lan@...ts.osuosl.org, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
"David S. Miller" <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH RESEND] igbvf: Fix rx_buffer_len
On 2023/01/24 19:49, Paul Menzel wrote:
> Dear Akihiko,
>
>
> Thank you for your patch.
>
> Am 24.01.23 um 05:39 schrieb Akihiko Odaki:
>
> Maybe improve the commit message summary to be more specific:
>
> igbvf: Align rx_buffer_len to fix memory corrption
>
>> When rx_buffer_len is not aligned by 1024, igbvf sets the aligned size
>> to SRRCTL while the buffer is allocated with the unaligned size. This
>> allows the device to write more data than rx_buffer_len, resulting in
>> memory corruption. Align rx_buffer_len itself so that the buffer will
>> be allocated with the aligned size.
>>
>> The condition to split RX packet header, which uses rx_buffer_len, is
>> also modified so that it doesn't change the behavior for the same
>> actual (unaligned) packet size. Actually the new condition is not
>> identical with the old one as it will no longer request splitting when
>> the actual packet size is exactly 2048, but that should be negligible.
>
> Is there an easy way to reproduce it?
>
>
> Kind regards,
>
> Paul
>
>
I withdraw this patch. While igbvf sets a value greater than the actual
buffer size to SRRCTL.BSIZEPKT, such a long packet should be dropped
according to VMOLR.RLPML.
Regards,
Akihiko Odaki
>> Fixes: d4e0fe01a38a ("igbvf: add new driver to support 82576 virtual
>> functions")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@...nix.com>
>> ---
>> drivers/net/ethernet/intel/igbvf/netdev.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c
>> b/drivers/net/ethernet/intel/igbvf/netdev.c
>> index 3a32809510fc..b6bca78198fa 100644
>> --- a/drivers/net/ethernet/intel/igbvf/netdev.c
>> +++ b/drivers/net/ethernet/intel/igbvf/netdev.c
>> @@ -1341,10 +1341,9 @@ static void igbvf_setup_srrctl(struct
>> igbvf_adapter *adapter)
>> srrctl |= E1000_SRRCTL_DROP_EN;
>> /* Setup buffer sizes */
>> - srrctl |= ALIGN(adapter->rx_buffer_len, 1024) >>
>> - E1000_SRRCTL_BSIZEPKT_SHIFT;
>> + srrctl |= adapter->rx_buffer_len >> E1000_SRRCTL_BSIZEPKT_SHIFT;
>> - if (adapter->rx_buffer_len < 2048) {
>> + if (adapter->rx_buffer_len <= 2048) {
>> adapter->rx_ps_hdr_size = 0;
>> srrctl |= E1000_SRRCTL_DESCTYPE_ADV_ONEBUF;
>> } else {
>> @@ -1625,7 +1624,7 @@ static int igbvf_sw_init(struct igbvf_adapter
>> *adapter)
>> struct net_device *netdev = adapter->netdev;
>> s32 rc;
>> - adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN + ETH_FCS_LEN;
>> + adapter->rx_buffer_len = ALIGN(ETH_FRAME_LEN + VLAN_HLEN +
>> ETH_FCS_LEN, 1024);
>> adapter->rx_ps_hdr_size = 0;
>> adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN;
>> adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
>> @@ -2429,6 +2428,8 @@ static int igbvf_change_mtu(struct net_device
>> *netdev, int new_mtu)
>> adapter->rx_buffer_len = ETH_FRAME_LEN + VLAN_HLEN +
>> ETH_FCS_LEN;
>> + adapter->rx_buffer_len = ALIGN(adapter->rx_buffer_len, 1024);
>> +
>> netdev_dbg(netdev, "changing MTU from %d to %d\n",
>> netdev->mtu, new_mtu);
>> netdev->mtu = new_mtu;
Powered by blists - more mailing lists