Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink interface

[Michal Kubecek <mkubecek@suse.cz>]

On Wed, Jul 03, 2019 at 12:04:35PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 06:34:37PM CEST, mkubecek@suse.cz wrote:
> >On Tue, Jul 02, 2019 at 03:05:15PM +0200, Jiri Pirko wrote:
> >> Tue, Jul 02, 2019 at 01:50:04PM CEST, mkubecek@suse.cz wrote:
> >> >+/**
> >> >+ * ethnl_is_privileged() - check if request has sufficient privileges
> >> >+ * @skb: skb with client request
> >> >+ *
> >> >+ * Checks if client request has CAP_NET_ADMIN in its netns. Unlike the flags
> >> >+ * in genl_ops, this allows finer access control, e.g. allowing or denying
> >> >+ * the request based on its contents or witholding only part of the data
> >> >+ * from unprivileged users.
> >> >+ *
> >> >+ * Return: true if request is privileged, false if not
> >> >+ */
> >> >+static inline bool ethnl_is_privileged(struct sk_buff *skb)
> >> 
> >> I wonder why you need this helper. Genetlink uses
> >> ops->flags & GENL_ADMIN_PERM for this. 
> >
> >It's explained in the function description. Sometimes we need finer
> >control than by request message type. An example is the WoL password:
> >ETHTOOL_GWOL is privileged because of it but I believe there si no
> >reason why unprivileged user couldn't see enabled WoL modes, we can
> >simply omit the password for him. (Also, it allows to combine query for
> >WoL settings with other unprivileged settings.)
> 
> Why can't we have rather:
> ETHTOOL_WOL_GET for all
> ETHTOOL_WOL_PASSWORD_GET  with GENL_ADMIN_PERM
> ?
> Better to stick with what we have in gennetlink rather then to bend the
> implementation from the very beginning I think.

We can. But it would also mean two separate SET requests (or breaking
the rule that _GET_REPLY, _SET and _NTF share the layout). That would be
unfortunate as ethtool_ops callback does not actually allow setting only
the modes so that the ETHTOOL_MSG_WOL_SET request (which would have to
go first as many drivers ignore .sopass if WAKE_MAGICSECURE is not set)
would have to pass a different password (most likely just leaving what
->get_wol() put there) and that password would be actually set until the
second request arrives. There goes the idea of getting rid of ioctl
interface raciness...

I would rather see returning to WoL modes not being visible to
unprivileged users than that (even if there is no actual reason for it).
Anyway, shortening the series left WoL settings out if the first part so
that I can split this out for now and leave the discussion for when we
get to WoL one day.

> >> >+/**
> >> >+ * ethnl_reply_header_size() - total size of reply header
> >> >+ *
> >> >+ * This is an upper estimate so that we do not need to hold RTNL lock longer
> >> >+ * than necessary (to prevent rename between size estimate and composing the
> >> 
> >> I guess this description is not relevant anymore. I don't see why to
> >> hold rtnl mutex for this function...
> >
> >You don't need it for this function, it's the other way around: unless
> >you hold RTNL lock for the whole time covering both checking needed
> >message size and filling the message - and we don't - the device could
> >be renamed in between. Thus if we returned size based on current device
> >name, it might not be sufficient at the time the header is filled.
> >That's why this function returns maximum possible size (which is
> >actually a constant).
> 
> I suggest to avoid the description. It is misleading. Perhaps something
> to have in a patch description but not here in code.

The reason I put the comment there was to prevent someone "optimizing"
the helper by using strlen() later. Maybe something shorter and more to
the point, e.g.

  Using IFNAMSIZ is faster and prevents a race if the device is renamed
  before we fill the name into skb.

?

Michal