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]
Message-Id: <20230125230519.1069676-1-kuba@kernel.org>
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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ