netdev - [PATCH net-next v2 0/2] ethtool: netlink: handle SET intro/outro in the common code

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

Date:   Wed, 25 Jan 2023 15:05:17 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
        Jakub Kicinski <kuba@...nel.org>
Subject: [PATCH net-next v2 0/2] ethtool: netlink: handle SET intro/outro in the common code

Factor out the boilerplate code from SET handlers to common code.

I volunteered to refactor the extack in GET in a conversation
with Vladimir but I gave up.

The handling of failures during dump in GET handlers is a bit
unclear to me. Some code uses presence of info as indication
of dump and tries to avoid reporting errors altogether
(including extack messages).

There's also the question of whether we should have a validation
callback (similar to .set_validate here) for GET. It looks like
.parse_request was expected to perform the validation. It takes
the extack and tb directly, not via info:

	int (*parse_request)(struct ethnl_req_info *req_info,
			     struct nlattr **tb,
			     struct netlink_ext_ack *extack);

	int (*prepare_data)(const struct ethnl_req_info *req_info,
			    struct ethnl_reply_data *reply_data,
			    struct genl_info *info);

so no crashes dereferencing info possible.

But .parse_request doesn't run under rtnl nor ethnl_ops_begin().
As a result some implementations defer validation until .prepare_data
where all the locks are held and they can call out to the driver.

All this makes me think that maybe we should refactor GET in the
same direction I'm refactoring SET. Split .prepare_data, take
more locks in the core, and add a validation helper which would
take extack directly:

    - ret = ops->prepare_data(req_info, reply_data, info);
    + ret = ops->prepare_data_validate(req_info, reply_data, attrs, extack);
    + if (ret < 1) // if 0 -> skip for dump; -EOPNOTSUPP in do
    +   goto err1;
    +
    + ret = ethnl_ops_begin(dev);
    + if (ret)
    +   goto err1;
    +
    + ret = ops->prepare_data(req_info, reply_data); // no extack
    + ethnl_ops_complete(dev);

I'll file that away as a TODO for posterity / older me.

v2:
 - invert checks for coalescing to avoid error code changes
 - rebase and convert MM as well

v1: https://lore.kernel.org/all/20230121054430.642280-1-kuba@kernel.org/

Jakub Kicinski (2):
  ethtool: netlink: handle SET intro/outro in the common code
  ethtool: netlink: convert commands to common SET

 net/ethtool/channels.c  |  92 ++++++++++++++----------------------
 net/ethtool/coalesce.c  |  92 ++++++++++++++++--------------------
 net/ethtool/debug.c     |  71 ++++++++++++----------------
 net/ethtool/eee.c       |  78 ++++++++++++-------------------
 net/ethtool/fec.c       |  83 +++++++++++++--------------------
 net/ethtool/linkinfo.c  |  81 ++++++++++++++------------------
 net/ethtool/linkmodes.c |  91 +++++++++++++++++-------------------
 net/ethtool/mm.c        |  82 ++++++++++++--------------------
 net/ethtool/module.c    |  89 ++++++++++++++---------------------
 net/ethtool/netlink.c   |  91 ++++++++++++++++++++++++++++++------
 net/ethtool/netlink.h   |  36 +++++++-------
 net/ethtool/pause.c     |  79 +++++++++++++------------------
 net/ethtool/plca.c      |  75 +++++++++--------------------
 net/ethtool/privflags.c |  84 ++++++++++++++++-----------------
 net/ethtool/pse-pd.c    |  79 ++++++++++++-------------------
 net/ethtool/rings.c     | 101 +++++++++++++++++-----------------------
 net/ethtool/wol.c       |  79 +++++++++++++------------------
 17 files changed, 608 insertions(+), 775 deletions(-)

-- 
2.39.1