Merge branch 'ethtool-netlink-next'
authorDavid S. Miller <davem@davemloft.net>
Fri, 27 Jan 2023 12:24:32 +0000 (12:24 +0000)
committerDavid S. Miller <davem@davemloft.net>
Fri, 27 Jan 2023 12:24:32 +0000 (12:24 +0000)
commit86e99b5bf2781735e3cc3d0fe3a57a9e6393e811
tree2bd61f7a84e4eaa7a42bc5b9f1249f3e9ccd9fb2
parentc766e077d927e1775902c18827205ea2ade3a35d
parent04007961bfaf76894b65de2af67f96d9d1fa82cf
Merge branch 'ethtool-netlink-next'

Jakub Kicinski says:

====================
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/
====================

Signed-off-by: David S. Miller <davem@davemloft.net>