mbox series

[net-next,v2,00/10] genetlink: support per-command policy dump

Message ID 20201001225933.1373426-1-kuba@kernel.org
Headers show
Series genetlink: support per-command policy dump | expand

Message

Jakub Kicinski Oct. 1, 2020, 10:59 p.m. UTC
Hi!

The objective of this series is to dump ethtool policies
to be able to tell which flags are supported by the kernel.
Current release adds ETHTOOL_FLAG_STATS for dumping extra
stats, but because of strict checking we need to make sure
that the flag is actually supported before setting it in
a request.

Ethtool policies are per command, and so far only dumping
family policies was supported.

The series adds new set of "light" ops to genl families which
don't have all the callbacks, and won't have the policy.
Most of families are then moved to these ops. This gives
us 4096B in savings on an allyesconfig build (not counting
the growth that would have happened when policy is added):

     text       data       bss        dec       hex
244415581  227958581  78372980  550747142  20d3bc06
244415581  227962677  78372980  550751238  20d3cc06

Next 6 patches deal the dumping per-op policy.

v2:
 - remove the stale comment in taskstats
 - split patch 8 -> 8, 9
 - now the getfamily policy is also in the op
 - make cmd u32
v1:
 - replace remaining uses of "light" with "small"
 - fix dump (ops can't be on the stack there)
 - coding changes in patch 4
 - new patch 7
 - don't echo op in responses - to make dump all easier

Dave - this series will cause a very trivial conflict with
the patch I sent to net. Both sides add some kdoc to struct
genl_ops so we'll need to keep it all.  I'm sending this
already because I also need to restructure ethool policies
in time for 5.10 if we want to use it for the stats flag.

Jakub Kicinski (10):
  genetlink: reorg struct genl_family
  genetlink: add small version of ops
  genetlink: move to smaller ops wherever possible
  genetlink: add a structure for dump state
  genetlink: use .start callback for dumppolicy
  genetlink: bring back per op policy
  taskstats: move specifying netlink policy back to ops
  genetlink: use parsed attrs in dumppolicy
  genetlink: switch control commands to per-op policies
  genetlink: allow dumping command-specific policy

 drivers/block/nbd.c                      |   6 +-
 drivers/net/gtp.c                        |   6 +-
 drivers/net/ieee802154/mac802154_hwsim.c |   6 +-
 drivers/net/macsec.c                     |   6 +-
 drivers/net/team/team.c                  |   6 +-
 drivers/net/wireless/mac80211_hwsim.c    |   6 +-
 drivers/target/target_core_user.c        |   6 +-
 drivers/thermal/thermal_netlink.c        |   8 +-
 fs/dlm/netlink.c                         |   6 +-
 include/net/genetlink.h                  |  67 +++++--
 include/net/netlink.h                    |   9 +-
 include/uapi/linux/genetlink.h           |   1 +
 kernel/taskstats.c                       |  40 +---
 net/batman-adv/netlink.c                 |   6 +-
 net/core/devlink.c                       |   6 +-
 net/core/drop_monitor.c                  |   6 +-
 net/hsr/hsr_netlink.c                    |   6 +-
 net/ieee802154/netlink.c                 |   6 +-
 net/ipv4/fou.c                           |   6 +-
 net/ipv4/tcp_metrics.c                   |   6 +-
 net/l2tp/l2tp_netlink.c                  |   6 +-
 net/mptcp/pm_netlink.c                   |   6 +-
 net/ncsi/ncsi-netlink.c                  |   6 +-
 net/netfilter/ipvs/ip_vs_ctl.c           |   6 +-
 net/netlabel/netlabel_calipso.c          |   6 +-
 net/netlabel/netlabel_cipso_v4.c         |   6 +-
 net/netlabel/netlabel_mgmt.c             |   6 +-
 net/netlabel/netlabel_unlabeled.c        |   6 +-
 net/netlink/genetlink.c                  | 234 ++++++++++++++++-------
 net/netlink/policy.c                     |  29 +--
 net/openvswitch/conntrack.c              |   6 +-
 net/openvswitch/datapath.c               |  24 +--
 net/openvswitch/meter.c                  |   6 +-
 net/psample/psample.c                    |   6 +-
 net/tipc/netlink_compat.c                |   6 +-
 net/wimax/stack.c                        |   6 +-
 net/wireless/nl80211.c                   |   5 +
 37 files changed, 346 insertions(+), 239 deletions(-)

Comments

Jakub Kicinski Oct. 2, 2020, 12:36 a.m. UTC | #1
On Thu,  1 Oct 2020 15:59:23 -0700 Jakub Kicinski wrote:
> Hi!
> 
> The objective of this series is to dump ethtool policies
> to be able to tell which flags are supported by the kernel.
> Current release adds ETHTOOL_FLAG_STATS for dumping extra
> stats, but because of strict checking we need to make sure
> that the flag is actually supported before setting it in
> a request.

Do we need support for separate .doit and .dumpit policies?
Or is that an overkill?
Johannes Berg Oct. 2, 2020, 6:29 a.m. UTC | #2
On Thu, 2020-10-01 at 17:36 -0700, Jakub Kicinski wrote:
> On Thu,  1 Oct 2020 15:59:23 -0700 Jakub Kicinski wrote:
> > Hi!
> > 
> > The objective of this series is to dump ethtool policies
> > to be able to tell which flags are supported by the kernel.
> > Current release adds ETHTOOL_FLAG_STATS for dumping extra
> > stats, but because of strict checking we need to make sure
> > that the flag is actually supported before setting it in
> > a request.
> 
> Do we need support for separate .doit and .dumpit policies?
> Or is that an overkill?

I suppose you could make an argument that only some attrs might be
accepted in doit and somewhat others in dumpit, or perhaps none in
dumpit because filtering wasn't implemented?

But still ... often we treat filtering as "advisory" anyway (except
perhaps where there's no doit at all, like the dump_policy thing here),
so it wouldn't matter if some attribute is ending up ignored?

johannes
Jakub Kicinski Oct. 2, 2020, 2:40 p.m. UTC | #3
On Fri, 02 Oct 2020 08:29:27 +0200 Johannes Berg wrote:
> On Thu, 2020-10-01 at 17:36 -0700, Jakub Kicinski wrote:
> > Do we need support for separate .doit and .dumpit policies?
> > Or is that an overkill?  
> 
> I suppose you could make an argument that only some attrs might be
> accepted in doit and somewhat others in dumpit, or perhaps none in
> dumpit because filtering wasn't implemented?

Right? Feels like it goes against our strict validation policy to
ignore input on dumpit.

> But still ... often we treat filtering as "advisory" anyway (except
> perhaps where there's no doit at all, like the dump_policy thing here),
> so it wouldn't matter if some attribute is ending up ignored?

It may be useful for feature discovery to know if an attribute is
supported.

I don't think it matters for any user right now, but maybe we should
require user space to specify if they are interested in normal req
policy or dump policy? That'd give us the ability to report different
ones in the future when the need arises.
Johannes Berg Oct. 2, 2020, 2:42 p.m. UTC | #4
On Fri, 2020-10-02 at 07:40 -0700, Jakub Kicinski wrote:

> > I suppose you could make an argument that only some attrs might be
> > accepted in doit and somewhat others in dumpit, or perhaps none in
> > dumpit because filtering wasn't implemented?
> 
> Right? Feels like it goes against our strict validation policy to
> ignore input on dumpit.
> 
> > But still ... often we treat filtering as "advisory" anyway (except
> > perhaps where there's no doit at all, like the dump_policy thing here),
> > so it wouldn't matter if some attribute is ending up ignored?
> 
> It may be useful for feature discovery to know if an attribute is
> supported.

Fair point.

> I don't think it matters for any user right now, but maybe we should
> require user space to specify if they are interested in normal req
> policy or dump policy? That'd give us the ability to report different
> ones in the future when the need arises.

Or just give them both? I mean, in many (most?) cases they're anyway
going to be the same, so with the patches I posted you could just give
them the two different policy indexes, and they can be the same?

But whichever, doesn't really matter much.

johannes
Jakub Kicinski Oct. 2, 2020, 2:55 p.m. UTC | #5
On Fri, 02 Oct 2020 16:42:09 +0200 Johannes Berg wrote:
> On Fri, 2020-10-02 at 07:40 -0700, Jakub Kicinski wrote:
> 
> > > I suppose you could make an argument that only some attrs might be
> > > accepted in doit and somewhat others in dumpit, or perhaps none in
> > > dumpit because filtering wasn't implemented?  
> > 
> > Right? Feels like it goes against our strict validation policy to
> > ignore input on dumpit.
> >   
> > > But still ... often we treat filtering as "advisory" anyway (except
> > > perhaps where there's no doit at all, like the dump_policy thing here),
> > > so it wouldn't matter if some attribute is ending up ignored?  
> > 
> > It may be useful for feature discovery to know if an attribute is
> > supported.  
> 
> Fair point.
> 
> > I don't think it matters for any user right now, but maybe we should
> > require user space to specify if they are interested in normal req
> > policy or dump policy? That'd give us the ability to report different
> > ones in the future when the need arises.  
> 
> Or just give them both? I mean, in many (most?) cases they're anyway
> going to be the same, so with the patches I posted you could just give
> them the two different policy indexes, and they can be the same?

Ah, I missed your posting! Like this?

[OP_POLICY]
   [OP]
      [DO]   -> u32
      [DUMP] -> u32

> But whichever, doesn't really matter much.
Johannes Berg Oct. 2, 2020, 2:58 p.m. UTC | #6
> > Or just give them both? I mean, in many (most?) cases they're anyway
> > going to be the same, so with the patches I posted you could just give
> > them the two different policy indexes, and they can be the same?
> 
> Ah, I missed your posting!

Huh, I even CC'ed you I think?

https://lore.kernel.org/netdev/20201002090944.195891-1-johannes@sipsolutions.net/t/#u

and userspace:

https://lore.kernel.org/netdev/20201002102609.224150-1-johannes@sipsolutions.net/t/#u

>  Like this?
> 
> [OP_POLICY]
>    [OP]
>       [DO]   -> u32
>       [DUMP] -> u32

Yeah, that'd work. I'd probably wonder if we shouldn't do

[OP_POLICY]
  [OP] -> (u32, u32)

in a struct with two u32's, since that's quite a bit more compact.

I did only:

[OP_POLICY]
  [OP] -> u32

johannes
Jakub Kicinski Oct. 2, 2020, 3:03 p.m. UTC | #7
On Fri, 02 Oct 2020 16:58:33 +0200 Johannes Berg wrote:
> > > Or just give them both? I mean, in many (most?) cases they're anyway
> > > going to be the same, so with the patches I posted you could just give
> > > them the two different policy indexes, and they can be the same?  
> > 
> > Ah, I missed your posting!  
> 
> Huh, I even CC'ed you I think?

I filter stuff which is to:netdev cc:me and get to it when I read the
ML. There's too much of it.

> https://lore.kernel.org/netdev/20201002090944.195891-1-johannes@sipsolutions.net/t/#u
> 
> and userspace:
> 
> https://lore.kernel.org/netdev/20201002102609.224150-1-johannes@sipsolutions.net/t/#u
> 
> >  Like this?
> > 
> > [OP_POLICY]
> >    [OP]
> >       [DO]   -> u32
> >       [DUMP] -> u32  
> 
> Yeah, that'd work. I'd probably wonder if we shouldn't do
> 
> [OP_POLICY]
>   [OP] -> (u32, u32)
> 
> in a struct with two u32's, since that's quite a bit more compact.

What do we do if the op doesn't have a dump or do callback?
0 is a valid policy ID, sadly :(
Johannes Berg Oct. 2, 2020, 3:04 p.m. UTC | #8
On Fri, 2020-10-02 at 08:03 -0700, Jakub Kicinski wrote:

> > Huh, I even CC'ed you I think?
> 
> I filter stuff which is to:netdev cc:me and get to it when I read the
> ML. There's too much of it.

Ah, ok :)

> > Yeah, that'd work. I'd probably wonder if we shouldn't do
> > 
> > [OP_POLICY]
> >   [OP] -> (u32, u32)
> > 
> > in a struct with two u32's, since that's quite a bit more compact.
> 
> What do we do if the op doesn't have a dump or do callback?
> 0 is a valid policy ID, sadly :(

Hm, good point. We could do -1 since that can't ever be reached though.

But compactness isn't really that necessary here anyway, so ...

johannes
Jakub Kicinski Oct. 2, 2020, 3:09 p.m. UTC | #9
On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote:
> > > Yeah, that'd work. I'd probably wonder if we shouldn't do
> > > 
> > > [OP_POLICY]
> > >   [OP] -> (u32, u32)
> > > 
> > > in a struct with two u32's, since that's quite a bit more compact.  
> > 
> > What do we do if the op doesn't have a dump or do callback?
> > 0 is a valid policy ID, sadly :(  
> 
> Hm, good point. We could do -1 since that can't ever be reached though.
> 
> But compactness isn't really that necessary here anyway, so ...

Cool, sounds like a plan.

This series should be good to merge, then.
Johannes Berg Oct. 2, 2020, 3:13 p.m. UTC | #10
On Fri, 2020-10-02 at 08:09 -0700, Jakub Kicinski wrote:
> On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote:
> > > > Yeah, that'd work. I'd probably wonder if we shouldn't do
> > > > 
> > > > [OP_POLICY]
> > > >   [OP] -> (u32, u32)
> > > > 
> > > > in a struct with two u32's, since that's quite a bit more compact.  
> > > 
> > > What do we do if the op doesn't have a dump or do callback?
> > > 0 is a valid policy ID, sadly :(  
> > 
> > Hm, good point. We could do -1 since that can't ever be reached though.
> > 
> > But compactness isn't really that necessary here anyway, so ...
> 
> Cool, sounds like a plan.
> 
> This series should be good to merge, then.

I suppose, I thought you wanted to change it to have separate dump/do
policies? Whatever you like there, I don't really care much :)

But I can also change my patches later to separately advertise dump/do
policies, and simply always use the same one for now.

But this series does conflict with the little bugfix I also sent, could
you please take a look?

https://lore.kernel.org/netdev/20201002094604.480c760e3c47.I7811da1539351a26cd0e5a10b98a8842cfbc1b55@changeid/

I'm not really sure how to handle.

Thanks,
johannes
Jakub Kicinski Oct. 2, 2020, 3:25 p.m. UTC | #11
On Fri, 02 Oct 2020 17:13:28 +0200 Johannes Berg wrote:
> On Fri, 2020-10-02 at 08:09 -0700, Jakub Kicinski wrote:
> > On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote:  
> > > > > Yeah, that'd work. I'd probably wonder if we shouldn't do
> > > > > 
> > > > > [OP_POLICY]
> > > > >   [OP] -> (u32, u32)
> > > > > 
> > > > > in a struct with two u32's, since that's quite a bit more compact.    
> > > > 
> > > > What do we do if the op doesn't have a dump or do callback?
> > > > 0 is a valid policy ID, sadly :(    
> > > 
> > > Hm, good point. We could do -1 since that can't ever be reached though.
> > > 
> > > But compactness isn't really that necessary here anyway, so ...  
> > 
> > Cool, sounds like a plan.
> > 
> > This series should be good to merge, then.  
> 
> I suppose, I thought you wanted to change it to have separate dump/do
> policies? Whatever you like there, I don't really care much :)

I just want to make the uAPI future-proof for now.

At a quick look ethtool doesn't really accept any attributes but
headers for GET requests. DO and DUMP are the same there so it's 
not a priority for me.

> But I can also change my patches later to separately advertise dump/do
> policies, and simply always use the same one for now.

Right that was what I was thinking. Basically:

	if ((op.doit && nla_put_u32(skb, CTRL_whatever_DO, idx)) ||
	    (op.dumpit && nla_put_u32(skb, CTRL_whatever_DUMP, idx)))
		goto nla_put_failure;

> But this series does conflict with the little bugfix I also sent, could
> you please take a look?
> 
> https://lore.kernel.org/netdev/20201002094604.480c760e3c47.I7811da1539351a26cd0e5a10b98a8842cfbc1b55@changeid/
> 
> I'm not really sure how to handle.

Yeah, just noticed that one now :S

Dave, are you planning a PR to Linus soon by any chance? The conflict
between this series and Johannes's fix would be logically simple to
resolve but not trivial :(
Johannes Berg Oct. 2, 2020, 3:28 p.m. UTC | #12
On Fri, 2020-10-02 at 08:25 -0700, Jakub Kicinski wrote:
> 
> > I suppose, I thought you wanted to change it to have separate dump/do
> > policies? Whatever you like there, I don't really care much :)
> 
> I just want to make the uAPI future-proof for now.

Yeah, makes sense.

> At a quick look ethtool doesn't really accept any attributes but
> headers for GET requests. DO and DUMP are the same there so it's 
> not a priority for me.

OK.

> > But I can also change my patches later to separately advertise dump/do
> > policies, and simply always use the same one for now.
> 
> Right that was what I was thinking. Basically:
> 
> 	if ((op.doit && nla_put_u32(skb, CTRL_whatever_DO, idx)) ||
> 	    (op.dumpit && nla_put_u32(skb, CTRL_whatever_DUMP, idx)))
> 		goto nla_put_failure;

Right, easy enough.

> > But this series does conflict with the little bugfix I also sent, could
> > you please take a look?
> > 
> > https://lore.kernel.org/netdev/20201002094604.480c760e3c47.I7811da1539351a26cd0e5a10b98a8842cfbc1b55@changeid/
> > 
> > I'm not really sure how to handle.
> 
> Yeah, just noticed that one now :S

Yeah, sorry ... The conflicts indeed weren't difficult, but non-trivial
unless you know what's going on in each side. I pushed them to my
mac80211-next tree, in the genetlink-op-policy-export branch (not sure
you saw my patches and cover letter yet :) )

I'll wait for this to get resolved and then respin my patches with the
above doit/dumpit after your series is in, and will also respin the
userspace side then.

johannes
Michal Kubecek Oct. 2, 2020, 4:55 p.m. UTC | #13
On Fri, Oct 02, 2020 at 08:25:17AM -0700, Jakub Kicinski wrote:
> On Fri, 02 Oct 2020 17:13:28 +0200 Johannes Berg wrote:

> > I suppose, I thought you wanted to change it to have separate dump/do

> > policies? Whatever you like there, I don't really care much :)

> 

> I just want to make the uAPI future-proof for now.

> 

> At a quick look ethtool doesn't really accept any attributes but

> headers for GET requests. DO and DUMP are the same there so it's 

> not a priority for me.


This is likely to change, for -x / --show-rxfh-indir / --show-rxfh we
will neeed to specify RSS context id. This is also an example where
different policy for doit and dumpit would make sense.

Michal
David Miller Oct. 2, 2020, 8:07 p.m. UTC | #14
From: Jakub Kicinski <kuba@kernel.org>
Date: Fri, 2 Oct 2020 08:25:17 -0700

> Dave, are you planning a PR to Linus soon by any chance? The conflict
> between this series and Johannes's fix would be logically simple to
> resolve but not trivial :(

Let me apply Johannes's fix to both net and net-next, and then you can
respin a v3 of this series on top of that.
Johannes Berg Oct. 2, 2020, 8:27 p.m. UTC | #15
On Fri, 2020-10-02 at 08:09 -0700, Jakub Kicinski wrote:
> On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote:
> > > > Yeah, that'd work. I'd probably wonder if we shouldn't do
> > > > 
> > > > [OP_POLICY]
> > > >   [OP] -> (u32, u32)
> > > > 
> > > > in a struct with two u32's, since that's quite a bit more compact.  
> > > 
> > > What do we do if the op doesn't have a dump or do callback?
> > > 0 is a valid policy ID, sadly :(  
> > 
> > Hm, good point. We could do -1 since that can't ever be reached though.
> > 
> > But compactness isn't really that necessary here anyway, so ...
> 
> Cool, sounds like a plan.
> 
> This series should be good to merge, then.

So I'm having second thoughts on this now :)

If you ask me to split the policy dump to do/dump, like we discussed
above, then what you did here for "retrieve a single policy" doesn't
really make any sense? Because you'd be able to do that properly only
for do, or you need my patches to get both?

Perhaps it would make sense if you removed patch 10 from your set, and
we add it back after my patches?

Or I could submit my patches right after yours, but that leaves the code
between the commits doing something weird, in that it would only give
you the policies but no indication of which is for do/dump? Obviously
today it'd only be one, but still, from a uAPI perspective.

I guess it doesn't matter too much though, we get to the state that we
want to be in, just the intermediate steps won't necessarily make much
sense.

For now I'll respin my patches so we see how the above do/dump
separating looks.

johannes
Jakub Kicinski Oct. 2, 2020, 8:50 p.m. UTC | #16
On Fri, 02 Oct 2020 22:27:19 +0200 Johannes Berg wrote:
> On Fri, 2020-10-02 at 08:09 -0700, Jakub Kicinski wrote:
> > On Fri, 02 Oct 2020 17:04:11 +0200 Johannes Berg wrote:  
> > > > > Yeah, that'd work. I'd probably wonder if we shouldn't do
> > > > > 
> > > > > [OP_POLICY]
> > > > >   [OP] -> (u32, u32)
> > > > > 
> > > > > in a struct with two u32's, since that's quite a bit more compact.    
> > > > 
> > > > What do we do if the op doesn't have a dump or do callback?
> > > > 0 is a valid policy ID, sadly :(    
> > > 
> > > Hm, good point. We could do -1 since that can't ever be reached though.
> > > 
> > > But compactness isn't really that necessary here anyway, so ...  
> > 
> > Cool, sounds like a plan.
> > 
> > This series should be good to merge, then.  
> 
> So I'm having second thoughts on this now :)
> 
> If you ask me to split the policy dump to do/dump, like we discussed
> above, then what you did here for "retrieve a single policy" doesn't
> really make any sense? Because you'd be able to do that properly only
> for do, or you need my patches to get both?
> 
> Perhaps it would make sense if you removed patch 10 from your set, and
> we add it back after my patches?
> 
> Or I could submit my patches right after yours, but that leaves the code
> between the commits doing something weird, in that it would only give
> you the policies but no indication of which is for do/dump? Obviously
> today it'd only be one, but still, from a uAPI perspective.

My thinking was that until kernel actually start using separate dump
policies user space can assume policy 0 is relevant. But yeah, merging
your changes first would probably be best.

> I guess it doesn't matter too much though, we get to the state that we
> want to be in, just the intermediate steps won't necessarily make much
> sense.
> 
> For now I'll respin my patches so we see how the above do/dump
> separating looks.

I, OTOH, am having second thoughts about not implementing separate
policies for dump right away, since Michal said he'll need them soon :)

Any ideas on how to do that cleanly? At some point it will make sense
to have dumps and doits in separate structures, as you said earlier,
but can we have "small" and "full" ops for both? That seems like too
much :/
Johannes Berg Oct. 2, 2020, 8:59 p.m. UTC | #17
On Fri, 2020-10-02 at 13:50 -0700, Jakub Kicinski wrote:
> 
> My thinking was that until kernel actually start using separate dump
> policies user space can assume policy 0 is relevant. But yeah, merging
> your changes first would probably be best.

Works for me. I have it based on yours. Just updated my branch (top
commit is 4d5045adfe90), but I'll probably only actually email it out
once things are a bit more settled wrt. your changes.

> I, OTOH, am having second thoughts about not implementing separate
> policies for dump right away, since Michal said he'll need them soon :)

:)

> Any ideas on how to do that cleanly? At some point it will make sense
> to have dumps and doits in separate structures, as you said earlier,
> but can we have "small" and "full" ops for both? That seems like too
> much :/

Not sure I understand what you just wrote :)

I had originally assumed dumps would be "infrequent", and so having the
small ops without dumps would be worthwhile. You said it wasn't true for
other users, so small ops still have .doit and .dumpit entries. Which is
fine?

But in the small ops anyway you don't have a policy pointer - I guess
you could have two "fallbacks" (for do and dump) in the family rather
than just one?

Another option - though it requires some rejiggering in my new policy
dump code - would be to key the lookup based on do/dump as well. Then
you could have the *same* op listed twice like

struct genl_ops my_ops[] = {
	{
		.cmd = SOMETHING,
		.doit = do_something,
		.policy = something_do_policy,
	},
	{
		.cmd = SOMETHING,
		.dumpit = dump_something,
		.policy = something_dump_policy,
	},
};

That way you only pay where needed? But ultimately with large ops you
already pay for the start/dump/done pointers, and you'd have that even
for the extra entry with _doit_ because ...

Unless we put three different kinds of ops (small, full-do, full-dump),
but that gets a bit awkward too?

johannes
Johannes Berg Oct. 2, 2020, 9 p.m. UTC | #18
On Fri, 2020-10-02 at 22:59 +0200, Johannes Berg wrote:
> On Fri, 2020-10-02 at 13:50 -0700, Jakub Kicinski wrote:
> > My thinking was that until kernel actually start using separate dump
> > policies user space can assume policy 0 is relevant. But yeah, merging
> > your changes first would probably be best.
> 
> Works for me. I have it based on yours. Just updated my branch (top
> commit is 4d5045adfe90), but I'll probably only actually email it out
> once things are a bit more settled wrt. your changes.

Forgot the link ...

https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export

johannes
Jakub Kicinski Oct. 2, 2020, 9:17 p.m. UTC | #19
On Fri, 02 Oct 2020 23:00:15 +0200 Johannes Berg wrote:
> On Fri, 2020-10-02 at 22:59 +0200, Johannes Berg wrote:
> > On Fri, 2020-10-02 at 13:50 -0700, Jakub Kicinski wrote:  
> > > My thinking was that until kernel actually start using separate dump
> > > policies user space can assume policy 0 is relevant. But yeah, merging
> > > your changes first would probably be best.  
> > 
> > Works for me. I have it based on yours. Just updated my branch (top
> > commit is 4d5045adfe90), but I'll probably only actually email it out
> > once things are a bit more settled wrt. your changes.  
> 
> Forgot the link ...
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export

If it's not too late for you - do you want to merge the two series and
post everything together? Perhaps squashing patch 10 into something if
that makes sense?

You already seem to have it rebased.
Jakub Kicinski Oct. 2, 2020, 9:22 p.m. UTC | #20
On Fri, 2 Oct 2020 14:17:01 -0700 Jakub Kicinski wrote:
> On Fri, 02 Oct 2020 23:00:15 +0200 Johannes Berg wrote:
> > On Fri, 2020-10-02 at 22:59 +0200, Johannes Berg wrote:  
> > > On Fri, 2020-10-02 at 13:50 -0700, Jakub Kicinski wrote:    
> > > > My thinking was that until kernel actually start using separate dump
> > > > policies user space can assume policy 0 is relevant. But yeah, merging
> > > > your changes first would probably be best.    
> > > 
> > > Works for me. I have it based on yours. Just updated my branch (top
> > > commit is 4d5045adfe90), but I'll probably only actually email it out
> > > once things are a bit more settled wrt. your changes.    
> > 
> > Forgot the link ...
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export  
> 
> If it's not too late for you - do you want to merge the two series and
> post everything together? Perhaps squashing patch 10 into something if
> that makes sense?
> 
> You already seem to have it rebased.

FWIW earlier I said:

	if ((op.doit && nla_put_u32(skb, CTRL_whatever_DO, idx)) ||
	    (op.dumpit && nla_put_u32(skb, CTRL_whatever_DUMP, idx)))
		goto nla_put_failure;

 - we should probably also check GENL_DONT_VALIDATE_DUMP here?
Jakub Kicinski Oct. 2, 2020, 9:52 p.m. UTC | #21
On Fri, 2 Oct 2020 14:22:05 -0700 Jakub Kicinski wrote:
> > > Forgot the link ...
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git/log/?h=genetlink-op-policy-export    
> > 
> > If it's not too late for you - do you want to merge the two series and
> > post everything together? Perhaps squashing patch 10 into something if
> > that makes sense?
> > 
> > You already seem to have it rebased.  

After double checking your tree has @policy in the wrong kdoc.

Let me post the first 9 patches and please squash patch 10 into your
series.

> FWIW earlier I said:
> 
> 	if ((op.doit && nla_put_u32(skb, CTRL_whatever_DO, idx)) ||
> 	    (op.dumpit && nla_put_u32(skb, CTRL_whatever_DUMP, idx)))
> 		goto nla_put_failure;
> 
>  - we should probably also check GENL_DONT_VALIDATE_DUMP here?