Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1293421

Re: [PATCH net-next v5 05/19] net: ethtool: add new ETHTOOL_GSETTINGS/SSETTINGS API

Show all headers | View raw

Thanks David: you are right, we should copy back sizeof(struct
ethtool_settings) in that case and not sizeof(usettings). Sorry about
that, will fix for v6.

a few questions before sending update:
 - is this handshake reasonable? or should we have an ethtool cmd
dedicated to this kind of handshake, or should we hardcode bitmap
length once and for all (ie. 128 instead of 32 today)?
 - is it ok to have the u32[] api part of bitmap.h? I was assuming it
could be used for other ioctl/syscalls outside ethtool... but maybe
this is being too pretentious and we could keep this internal to
net/core?
 - struct inheritance is used to have the link mode bitmaps
piggybacked at end the public struct ethtool_settings. hence this
"parent" field. I'm not super proud of this, but I find relying on the
compiler more comfortable. could revise my position with a
constructor+accessors macros if you think it's preferred.


On Wed, Dec 16, 2015 at 8:38 AM, David Miller <davem@davemloft.net> wrote:
> From: David Decotigny <ddecotig@gmail.com>
> Date: Mon, 14 Dec 2015 13:03:52 -0800
>
>> +static int ethtool_get_ksettings(struct net_device *dev, void __user *useraddr)
>> +{
>  ...
>> +     if (__ETHTOOL_LINK_MODE_MASK_NU32
>> +         != ksettings.parent.link_mode_masks_nwords) {
>> +             /* wrong link mode nbits requested */
>> +             memset(&ksettings, 0, sizeof(ksettings));
>> +             /* keep cmd field reset to 0 */
>> +             /* send back number of words required as negative val */
>> +             compiletime_assert(__ETHTOOL_LINK_MODE_MASK_NU32 <= S8_MAX,
>> +                                "need too many bits for link modes!");
>> +             ksettings.parent.link_mode_masks_nwords
>> +                     = -((s8)__ETHTOOL_LINK_MODE_MASK_NU32);
>
> I'm trying to understand how this can work.
>
> Supposedly, the link_mode_masks_nwords field is there so that we can
> add new link modes yet still work with tools built against any
> particular link mode list in the UAPI header files.
>
> But here you're forcing the value of link_mode_masks_nwords and then
> copying that amount back to userspace.  If the user allocated less
> space than the the link mode list in the kernel supports, we will
> overwrite past the end of the user's usettings object.
>
> You cannot unconditionally copy sizeof(usettings) back to the user,
> as store_ksettings_for_user() will do.
>
> I think you have to truncate here, copying only the array elements the
> user's structure actually has space for.  That's the only way this can
> work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH net-next v5 00/19] new ETHTOOL_GSETTINGS/SSETTINGS API David Decotigny <ddecotig@gmail.com> - 2015-12-14 22:10 +0100
  [PATCH net-next v5 19/19] net: mlx4: use new ETHTOOL_G/SSETTINGS API David Decotigny <ddecotig@gmail.com> - 2015-12-14 22:10 +0100
  [PATCH net-next v5 17/19] net: ethtool: remove unused __ethtool_get_settings David Decotigny <ddecotig@gmail.com> - 2015-12-14 22:10 +0100
  [PATCH net-next v5 02/19] test_bitmap: unit tests for lib/bitmap.c David Decotigny <ddecotig@gmail.com> - 2015-12-14 22:20 +0100
  [PATCH net-next v5 03/19] net: usnic: remove unused call to ethtool_ops::get_settings David Decotigny <ddecotig@gmail.com> - 2015-12-14 22:20 +0100
  [PATCH net-next v5 07/19] net: usnic: use __ethtool_get_ksettings David Decotigny <ddecotig@gmail.com> - 2015-12-14 22:20 +0100
  [PATCH net-next v5 06/19] tx4939: use __ethtool_get_ksettings David Decotigny <ddecotig@gmail.com> - 2015-12-14 22:20 +0100
  Re: [PATCH net-next v5 05/19] net: ethtool: add new  ETHTOOL_GSETTINGS/SSETTINGS API David Miller <davem@davemloft.net> - 2015-12-16 17:40 +0100
    Re: [PATCH net-next v5 05/19] net: ethtool: add new  ETHTOOL_GSETTINGS/SSETTINGS API David Decotigny <ddecotig@gmail.com> - 2015-12-17 01:30 +0100

csiph-web