From: Russell King <hidden> Date: 2020-08-05 19:22:40
syzbot has revealed that the "phylink" keyword exists in non-phylink
related contexts in the bluetooth stack. To avoid receiving
inappropriate notifications, change the keyword matching regexp to
something which avoids this, while still allowing changes to networking
drivers that make use of phylink to be detected.
Signed-off-by: Russell King <redacted>
---
Linus,
Is this something you're willing to merge directly please?
Thanks.
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Isn't the 'F' pattern already complete enough that maybe the K pattern
isn't even worth it?
Just a thought, no biggie.
Linus
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Yes, it's extremely awkward - I spent much of the morning with perl
testing it out on the drivers/ subtree.
Isn't the 'F' pattern already complete enough that maybe the K pattern
isn't even worth it?
Unfortunately not; I used not to have a K: line, which presented the
problem that we had users of phylink added to the kernel that were not
being reviewed. So, the suggestion was to add a K: line.
However, I'm now being spammed by syzbot (I've received multiple emails
about the same problem) because, rather than MAINTAINERS being applied
to just patches, it is now being applied to entire source files. This
means that the previous "K: phylink" entry matches not just on patches
(which can be easily ignored) but entire files, such as
net/bluetooth/hci_event.c which happens to contain "phylink" in a
function name.
So, when syzbot identifies there is a problem in
net/bluetooth/hci_event.c, it sends me a report, despite it having
no relevance for me.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Yes, it's extremely awkward - I spent much of the morning with perl
testing it out on the drivers/ subtree.
And perhaps easier to read would be to use multiple K: lines.
(?: used to avoid unnecessary capture groups)
K: phylink\.h|struct\s+phylink
K: (?:\.|\-\>)phylink_
K: phylink_(?:autoneg|clear|connect|create|destroy|disconnect|ethtool|helper|mac|mii|of|set|start|stop|test|validate)
Isn't the 'F' pattern already complete enough that maybe the K pattern
isn't even worth it?
Unfortunately not; I used not to have a K: line, which presented the
problem that we had users of phylink added to the kernel that were not
being reviewed. So, the suggestion was to add a K: line.
However, I'm now being spammed by syzbot (I've received multiple emails
about the same problem) because, rather than MAINTAINERS being applied
to just patches, it is now being applied to entire source files. This
means that the previous "K: phylink" entry matches not just on patches
(which can be easily ignored) but entire files, such as
net/bluetooth/hci_event.c which happens to contain "phylink" in a
function name.
So, when syzbot identifies there is a problem in
net/bluetooth/hci_event.c, it sends me a report, despite it having
no relevance for me.
Maybe instead syzbot could ignore K: lines
by adding --nokeywords to its command line
for get_maintainer.pl.
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Yes, it's extremely awkward - I spent much of the morning with perl
testing it out on the drivers/ subtree.
There are a lot of phylink_<foo> in the kernel.
Are those really the only uses you want to watch?
Hi Joe
I think Rusells intention here is to match on MAC drivers which make
use of the phylink API exported to them.
SFF/SFP/SFP+ MODULE SUPPORT
M: Russell King [off-list ref]
L: netdev@vger.kernel.org
S: Maintained
F: drivers/net/phy/phylink.c
F: drivers/net/phy/sfp*
F: include/linux/phylink.h
F: include/linux/sfp.h
K: phylink
$ git grep -P -oh 'phylink_\w+'| sort | uniq -c
Try that again, but skip files matched by the F: clauses.
I suspect the matches you get then more closely approximates the K:
Russell is suggesting.
Andrew
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Yes, it's extremely awkward - I spent much of the morning with perl
testing it out on the drivers/ subtree.
And perhaps easier to read would be to use multiple K: lines.
(?: used to avoid unnecessary capture groups)
K: phylink\.h|struct\s+phylink
K: (?:\.|\-\>)phylink_
That one is definitely incorrect. It is not .phylink_ or ->phylink_,
it was .phylink (without _) or >phylink_
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Yes, it's extremely awkward - I spent much of the morning with perl
testing it out on the drivers/ subtree.
There are a lot of phylink_<foo> in the kernel.
Are those really the only uses you want to watch?
It is sufficient; as I said, I've spent a morning running this:
#!/usr/bin/perl
$re = 'phylink\.h|struct\s+phylink|\.phylink|>phylink_|phylink_(autoneg|clear|connect|create|destroy|disconnect|ethtool|helper|mac|mii|of|set|start|stop|test|validate)';
foreach $f (@ARGV) {
open F, $f;
$l = 1;
while (<F>) {
chomp;
print "$f:$l: $_\n" if /$re/;
$l++;
}
close F;
}
through:
$ find drivers -type f -print0 | xargs -0 ./check.pl | diff -u pl-ref.out - |less
where pl-ref.out is the original K: matching of just "phylink" and
looking at the differences to ensure I'm excluding just stuff that
doesn't concern me, while getting a high hit rate on the stuff
that I do want.
Now, I'm not saying that there isn't a better way, but this is not
something I want to spend days on. So I got something that works
for me, and that's what I've sent Linus.
Going through your list...
4 phylink_add
Not sure what this is. Doesn't seem to be anything to do with what
I maintain.
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Yes, it's extremely awkward - I spent much of the morning with perl
testing it out on the drivers/ subtree.
And perhaps easier to read would be to use multiple K: lines.
(?: used to avoid unnecessary capture groups)
K: phylink\.h|struct\s+phylink
K: (?:\.|\-\>)phylink_
That one is definitely incorrect. It is not .phylink_ or ->phylink_,
it was .phylink (without _) or >phylink_
Hi Russell.
I don't see the difference.
All uses of .phylink are followed with _
as far as I can tell.
$ git grep -Poh "\.phylink\S*"|sort|uniq -c
1 .phylink_fixed_state
2 .phylink_mac_an_restart
9 .phylink_mac_config
1 .phylink_mac_config.
11 .phylink_mac_link_down
6 .phylink_mac_link_state
9 .phylink_mac_link_up
38 .phylink_validate
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Yes, it's extremely awkward - I spent much of the morning with perl
testing it out on the drivers/ subtree.
And perhaps easier to read would be to use multiple K: lines.
(?: used to avoid unnecessary capture groups)
K: phylink\.h|struct\s+phylink
K: (?:\.|\-\>)phylink_
That one is definitely incorrect. It is not .phylink_ or ->phylink_,
it was .phylink (without _) or >phylink_
Hi Russell.
I don't see the difference.
All uses of .phylink are followed with _
as far as I can tell.
$ git grep -Poh "\.phylink\S*"|sort|uniq -c
1 .phylink_fixed_state
2 .phylink_mac_an_restart
9 .phylink_mac_config
1 .phylink_mac_config.
11 .phylink_mac_link_down
6 .phylink_mac_link_state
9 .phylink_mac_link_up
38 .phylink_validate
Yes, you're right, but as I explained, I got something that works for
me, and I wasn't going to put more effort in.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
That's a very awkward pattern. I wonder if there could be better ways
to express this (ie "only apply this pattern to these files" kind of
thing)
Yes, it's extremely awkward - I spent much of the morning with perl
testing it out on the drivers/ subtree.
There are a lot of phylink_<foo> in the kernel.
Are those really the only uses you want to watch?
It is sufficient; as I said, I've spent a morning running this:
Cool for you, I was just asking.
How you determine what functions you're interested in
is up to you.
Another mechanism might have been something like:
$ git grep -Poh '(?:static\s+inline|EXPORT_SYMBOL).*phylink_[a-z]+' | \
grep -Poh 'phylink_[a-z]+' | sort | uniq
Maybe something that could be made generic in get_maintainer
for people maintaining specific EXPORT_SYMBOL(foo) blocks.
Maybe another letter/look could be added to MAINTAINERS like:
E: symbol_regex
as another similar mechanism for keywords but just for data
or functions marked as exported symbols.
For example, that E: might help minimize xdp matches as xdp
is used by many other bits of code that aren't express data
path uses.
MAINTAINERS-XDP (eXpress Data Path)
MAINTAINERS-M: Alexei Starovoitov [off-list ref]
MAINTAINERS-M: Daniel Borkmann [off-list ref]
MAINTAINERS-M: David S. Miller [off-list ref]
MAINTAINERS-M: Jakub Kicinski [off-list ref]
MAINTAINERS-M: Jesper Dangaard Brouer [off-list ref]
MAINTAINERS-M: John Fastabend [off-list ref]
MAINTAINERS-L: netdev@vger.kernel.org
MAINTAINERS-L: bpf@vger.kernel.org
MAINTAINERS-S: Supported
MAINTAINERS-F: include/net/xdp.h
MAINTAINERS-F: include/trace/events/xdp.h
MAINTAINERS-F: kernel/bpf/cpumap.c
MAINTAINERS-F: kernel/bpf/devmap.c
MAINTAINERS-F: net/core/xdp.c
MAINTAINERS-N: xdp
MAINTAINERS:K: xdp