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

2015-12-16 Thread David Decotigny
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  wrote:
> From: David Decotigny 
> 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 netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-12-16 Thread David Miller
From: David Decotigny 
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 netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2015-12-14 Thread David Decotigny
From: David Decotigny 

This patch defines a new ETHTOOL_GSETTINGS/SSETTINGS API, handled by
the new get_ksettings/set_ksettings callbacks. This API provides
support for most legacy ethtool_cmd fields, adds support for larger
link mode masks (up to 4064 bits, variable length), and removes
ethtool_cmd deprecated fields (transceiver/maxrxpkt/maxtxpkt).

This API is deprecating the legacy ETHTOOL_GSET/SSET API and provides
the following backward compatibility properties:
 - legacy ethtool with legacy drivers: no change, still using the
   get_settings/set_settings callbacks.
 - legacy ethtool with new get/set_ksettings drivers: the new driver
   callbacks are used, data internally converted to legacy
   ethtool_cmd. ETHTOOL_GSET will return only the 1st 32b of each link
   mode mask. ETHTOOL_SSET will fail if user tries to set the
   ethtool_cmd deprecated fields to non-0
   (transceiver/maxrxpkt/maxtxpkt). A kernel warning is logged if
   driver sets higher bits.
 - future ethtool with legacy drivers: no change, still using the
   get_settings/set_settings callbacks, internally converted to new data
   structure. Deprecated fields (transceiver/maxrxpkt/maxtxpkt) will be
   ignored and seen as 0 from user space. Note that that "future"
   ethtool tool will not allow changes to these deprecated fields.
 - future ethtool with new drivers: direct call to the new callbacks.

By "future" ethtool, what is meant is:
 - query: first try ETHTOOL_GSETTINGS, and revert to ETHTOOL_GSET if fails
 - set: query first and remember which of ETHTOOL_GSETTINGS or
   ETHTOOL_GSET was successful
   - if ETHTOOL_GSETTINGS was successful, then change config with
 ETHTOOL_SSETTINGS. A failure there is final (do not try ETHTOOL_SSET).
   - otherwise ETHTOOL_GSET was successful, change config with
 ETHTOOL_SSET. A failure there is final (do not try ETHTOOL_SSETTINGS).

The interaction user/kernel via the new API requires a small
ETHTOOL_GSETTINGS handshake first to agree on the length of the link
mode bitmaps. If kernel doesn't agree with user, it returns the bitmap
length it is expecting from user as a negative length (and cmd field
is 0). When kernel and user agree, kernel returns valid info in all
fields (ie. link mode length > 0 and cmd is ETHTOOL_GSETTINGS).

Data structure crossing user/kernel boundary is 32/64-bit
agnostic. Converted internally to a legal kernel bitmap.

The internal __ethtool_get_settings kernel helper will gradually be
replaced by __ethtool_get_ksettings by the time the first ksettings
drivers start to appear. So this patch doesn't change it, it will be
removed before it needs to be changed.

Signed-off-by: David Decotigny 
---
 include/linux/ethtool.h  |  88 +-
 include/uapi/linux/ethtool.h | 320 +++---
 net/core/ethtool.c   | 407 ++-
 3 files changed, 735 insertions(+), 80 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 653dc9c..6077cbb 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -12,6 +12,7 @@
 #ifndef _LINUX_ETHTOOL_H
 #define _LINUX_ETHTOOL_H
 
+#include 
 #include 
 #include 
 
@@ -40,9 +41,6 @@ struct compat_ethtool_rxnfc {
 
 #include 
 
-extern int __ethtool_get_settings(struct net_device *dev,
- struct ethtool_cmd *cmd);
-
 /**
  * enum ethtool_phys_id_state - indicator state for physical identification
  * @ETHTOOL_ID_INACTIVE: Physical ID indicator should be deactivated
@@ -97,13 +95,72 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 
n_rx_rings)
return index % n_rx_rings;
 }
 
+/* number of link mode bits/ulongs handled internally by kernel */
+#define __ETHTOOL_LINK_MODE_MASK_NBITS \
+   (__ETHTOOL_LINK_MODE_LAST + 1)
+
+/* declare a link mode bitmap */
+#define __ETHTOOL_DECLARE_LINK_MODE_MASK(name) \
+   DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
+
+/* drivers must ignore parent.cmd and parent.link_mode_masks_nwords
+ * fields, but they are allowed to overwrite them (will be ignored).
+ */
+struct ethtool_ksettings {
+   struct ethtool_settings parent;
+   struct {
+   __ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+   __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
+   __ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
+   } link_modes;
+};
+
+/**
+ * ethtool_ksettings_zero_link_mode - clear ksettings link mode mask
+ *   @ptr : pointer to struct ethtool_ksettings
+ *   @name : one of supported/advertising/lp_advertising
+ */
+#define ethtool_ksettings_zero_link_mode(ptr, name)\
+   bitmap_zero((ptr)->link_modes.name, __ETHTOOL_LINK_MODE_MASK_NBITS)
+
+/**
+ * ethtool_ksettings_add_link_mode - set bit in ksettings link mode mask
+ *   @ptr : pointer to struct ethtool_ksettings
+ *   @name : one of supported/advertising/lp_advertising
+ *   @mode : one of the ETHTOOL_LINK_MODE_