netdev - Re: [RFC PATCH] 8139cp: properly support change of MTU values

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] [day] [month] [year] [list]

Message-ID: <50B65121.2000803@redhat.com>
Date:	Wed, 28 Nov 2012 13:00:01 -0500
From:	John Greene <jogreene@...hat.com>
To:	Rami Rosen <roszenrami@...il.com>
CC:	netdev@...r.kernel.org
Subject: Re: [RFC PATCH] 8139cp: properly support change of MTU values

On 11/28/2012 02:23 AM, Rami Rosen wrote:
> Hi,
>
> In cp_change_mtu(), there is the following check:
> ...
> if (new_mtu < CP_MIN_MTU || new_mtu > CP_MAX_MTU)
> 		return -EINVAL;
> ...
> Later on, we set dev->mtu to new_mtu.
>
> The CP_MIN_MTU is defined to be 60; shouldn't it be 68 ?
>
>
> The reason for 68 is (RFC 791,  Internet Protocol,
> http://www.ietf.org/rfc/rfc791.txt):
> "Every internet module must be able to forward a datagram of 68 octets
> without further fragmentation.  This is because an internet  header
> may be up to 60 octets, and the minimum fragment is 8 octets."
>
> See also the generic Ethernet () method in eth_change_mtu() (net/ethernet/eth.c)
>
> int eth_change_mtu(struct net_device *dev, int new_mtu)
> {
> 	if (new_mtu < 68 || new_mtu > ETH_DATA_LEN)
> 		return -EINVAL;
> 	dev->mtu = new_mtu;
> 	return 0;
> }
>
>
> regards,
> Rami Rosen
>
> http://ramirose.wix.com/ramirosen
>
> On Tue, Nov 27, 2012 at 10:08 PM, John Greene <jogreene@...hat.com> wrote:
>> The 8139cp driver has a change_mtu function that has not been
>> enabled since the dawn of the git repository. However, the
>> generic eth_change_mtu is not used in its place, so that
>> invalid MTU values can be set on the interface.
>>
>> Original patch salvages the broken code for the single case of
>> setting the MTU while the interface is down, which is safe
>> and also includes the range check.  Now enhanced to support up
>> or down interface.
>>
>> Original patch from
>> http://lkml.indiana.edu/hypermail/linux/kernel/1202.2/00770.html
>>
>> Testing: has been test on virtual 8139cp setup without issue,
>> awaiting real hardware and retest again, but wanted to post now.
>>
>> Signed-Off-By: "John Greene" <jogreene@...hat.com>
>> CC: "David S. Miller" <davem@...emloft.net>commit c3f214170cd1abb89290815c3894b945a86894fe
Author: John Greene <jogreene@...hat.com>
Date:   Mon Nov 26 16:05:06 2012 -0500

     8139cp: properly support change of MTU values

>> ---
>>   drivers/net/ethernet/realtek/8139cp.c | 22 +++-------------------
>>   1 file changed, 3 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
>> index 6cb96b4..7847c83 100644
>> --- a/drivers/net/ethernet/realtek/8139cp.c
>> +++ b/drivers/net/ethernet/realtek/8139cp.c
>> @@ -1226,12 +1226,9 @@ static void cp_tx_timeout(struct net_device *dev)
>>          spin_unlock_irqrestore(&cp->lock, flags);
>>   }
>>
>> -#ifdef BROKEN
>>   static int cp_change_mtu(struct net_device *dev, int new_mtu)
>>   {
>>          struct cp_private *cp = netdev_priv(dev);
>> -       int rc;
>> -       unsigned long flags;
>>
>>          /* check for invalid MTU, according to hardware limits */
>>          if (new_mtu < CP_MIN_MTU || new_mtu > CP_MAX_MTU)
>> @@ -1244,22 +1241,11 @@ static int cp_change_mtu(struct net_device *dev, int new_mtu)
>>                  return 0;
>>          }
>>
>> -       spin_lock_irqsave(&cp->lock, flags);
>> -
>> -       cp_stop_hw(cp);                 /* stop h/w and free rings */
>> -       cp_clean_rings(cp);
>> -
>> +       /* network IS up, close it, reset MTU, and come up again. */
>> +       cp_close(dev);
>>          dev->mtu = new_mtu;
>> -       cp_set_rxbufsize(cp);           /* set new rx buf size */
>> -
>> -       rc = cp_init_rings(cp);         /* realloc and restart h/w */
>> -       cp_start_hw(cp);
>> -
>> -       spin_unlock_irqrestore(&cp->lock, flags);
>> -
>> -       return rc;
>> +       return cp_open(dev);
>>   }
>> -#endif /* BROKEN */
>>
>>   static const char mii_2_8139_map[8] = {
>>          BasicModeCtrl,
>> @@ -1835,9 +1821,7 @@ static const struct net_device_ops cp_netdev_ops = {
>>          .ndo_start_xmit         = cp_start_xmit,
>>          .ndo_tx_timeout         = cp_tx_timeout,
>>          .ndo_set_features       = cp_set_features,
>> -#ifdef BROKEN
>>          .ndo_change_mtu         = cp_change_mtu,
>> -#endif
>>
>>   #ifdef CONFIG_NET_POLL_CONTROLLER
>>          .ndo_poll_controller    = cp_poll_controller,
>> --
>> 1.7.11.7
>>
>> --
>> 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
Good catch Rami.  Thanks.

--
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