Mon, Jul 08, 2019 at 02:22:51PM CEST, [email protected] wrote: >On Wed, Jul 03, 2019 at 12:04:35PM +0200, Jiri Pirko wrote: >> Tue, Jul 02, 2019 at 06:34:37PM CEST, [email protected] wrote: >> >On Tue, Jul 02, 2019 at 03:05:15PM +0200, Jiri Pirko wrote: >> >> Tue, Jul 02, 2019 at 01:50:04PM CEST, [email protected] 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 understand. That is my concern, not to bring baggage from ioclt :/ > >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. Fine. > >> >> >+/** >> >> >+ * 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. > >? Sounds good, thanks! > >Michal
