* [PATCH] netfilter: conntrack: fix calculation of next bucket number in early_drop
From: Vasily Khoruzhick @ 2018-10-25 3:48 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S. Miller, netfilter-devel, coreteam, netdev, linux-kernel,
Dmitry Safonov
Cc: Vasily Khoruzhick, stable
If there's no entry to drop in bucket that corresponds to the hash,
early_drop() should look for it in other buckets. But since it increments
hash instead of bucket number, it actually looks in the same bucket 8
times: hsize is 16k by default (14 bits) and hash is 32-bit value, so
reciprocal_scale(hash, hsize) returns the same value for hash..hash+7 in
most cases.
Fix it by increasing bucket number instead of hash and rename _hash
to bucket to avoid future confusion.
Fixes: 3e86638e9a0b ("netfilter: conntrack: consider ct netns in early_drop logic")
Cc: <stable@vger.kernel.org> # v4.7+
Signed-off-by: Vasily Khoruzhick <vasilykh@arista.com>
---
net/netfilter/nf_conntrack_core.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index ca1168d67fac..a04af246b184 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1073,19 +1073,22 @@ static unsigned int early_drop_list(struct net *net,
return drops;
}
-static noinline int early_drop(struct net *net, unsigned int _hash)
+static noinline int early_drop(struct net *net, unsigned int hash)
{
unsigned int i;
for (i = 0; i < NF_CT_EVICTION_RANGE; i++) {
struct hlist_nulls_head *ct_hash;
- unsigned int hash, hsize, drops;
+ unsigned int bucket, hsize, drops;
rcu_read_lock();
nf_conntrack_get_ht(&ct_hash, &hsize);
- hash = reciprocal_scale(_hash++, hsize);
+ if (!i)
+ bucket = reciprocal_scale(hash, hsize);
+ else
+ bucket = (bucket + 1) % hsize;
- drops = early_drop_list(net, &ct_hash[hash]);
+ drops = early_drop_list(net, &ct_hash[bucket]);
rcu_read_unlock();
if (drops) {
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] Change judgment len position
From: Willy Tarreau @ 2018-10-25 3:20 UTC (permalink / raw)
To: Joe Perches
Cc: Al Viro, Fengguang Wu, David Miller, wanghaifine, netdev, LKML,
LKP, Philip Li
In-Reply-To: <4664b8b350ed35ee24746fd34fb0e600ced776a5.camel@perches.com>
On Wed, Oct 24, 2018 at 07:41:52PM -0700, Joe Perches wrote:
> The Code of Conduct, if it exists at all, should apply
> to all of the kernel.
>
> And no, as I have previously posted, I don't agree with
> it nor the method that was used to introduce it.
>
> But it does exist.
> Its splatter affects us all.
Then just like any rule, start to use it as a guideline and not as
something extremely strict to apply to others. If someone feels
offended he can complain, there's no reason for suggesting that
maybe someone else could have felt offended, otherwise we'll end
up with a new code of thinking to explain how to think what others
could feel like...
Willy
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Joe Perches @ 2018-10-25 2:41 UTC (permalink / raw)
To: Al Viro
Cc: Fengguang Wu, Willy Tarreau, David Miller, wanghaifine, netdev,
LKML, LKP, Philip Li
In-Reply-To: <20181025022027.GG32577@ZenIV.linux.org.uk>
On Thu, 2018-10-25 at 03:20 +0100, Al Viro wrote:
> On Wed, Oct 24, 2018 at 06:16:31PM -0700, Joe Perches wrote:
> > On Thu, 2018-10-25 at 09:11 +0800, Fengguang Wu wrote:
> > > CC Philip and LKP team.
> > > > Please try to make your first patch in drivers/staging
> > > > to get familiar with submitting patches to the kernel.
> > > > https://kernelnewbies.org/FirstKernelPatch
> > > >
> > > > Maybe there's utility in creating a filtering and auto-reply
> > > > tool for first time patch submitters for all the vger mailing
> > > > lists using some combination of previously known submitters
> > > > and the 0-day robot to point those first time submitters of
> > > > defective patches to kernelnewbies and staging.
> > >
> > > Yeah good idea. That feature can be broken into 2 parts:
> > >
> > > - an email script, which could be added to Linux scripts/ dir
> > > - maintain records for telling whether someone is first-time patch submitters
> >
> > Maybe run checkpatch on those first-time submitter patches too.
>
> OK, now I'm certain that you are trolling...
Nope, the process suggestions above are sincere.
> Joe, what really pisses me off is that it's actually at the expense of original
> poster (who had nothing to do with the CoCup)
CoCup? No doubt pronounced cock-up.
> *and* an invitation for a certain
> variety of kooks. In probably vain hope of heading that off, here's the
> summary of what happened _before_ Joe started to stir the shit:
>
> * code in question is, indeed, (slightly) bogus in mainline.
> It reads as "reject negative values for length, truncate positive ones to 4",
> but in reality it's "treat everything outside of 0..4 as 4". It's not broken
> per se, but it's certainly misleading.
> * one possible fix would be to drop the "reject negative values"
> completely, another - to move checking for negatives before the truncation.
> Patch tried to do the latter.
Umm, I suggested an appropriate mechanism to fix the patch
in this thread immediately after reading it.
> Code of Conduct is garbage, but neither Dave nor the author
> of this patch had anything to do with that mess. If you want to make a point,
> do so without shit splatter hitting innocent bystanders - people tend to
> get very annoyed by that kind of thing, and with a damn good reason.
The Code of Conduct, if it exists at all, should apply
to all of the kernel.
And no, as I have previously posted, I don't agree with
it nor the method that was used to introduce it.
But it does exist.
Its splatter affects us all.
^ permalink raw reply
* Re: [PATCH v2 bpf] bpf: devmap: fix wrong interface selection in notifier_call
From: Song Liu @ 2018-10-24 17:56 UTC (permalink / raw)
To: ap420073; +Cc: Daniel Borkmann, Alexei Starovoitov, Networking, John Fastabend
In-Reply-To: <20181024111517.13361-1-ap420073@gmail.com>
On Wed, Oct 24, 2018 at 4:16 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> The dev_map_notification() removes interface in devmap if
> unregistering interface's ifindex is same.
> But only checking ifindex is not enough because other netns can have
> same ifindex. so that wrong interface selection could occurred.
> Hence netdev pointer comparison code is added.
>
> v2: compare netdev pointer instead of using net_eq() (Daniel Borkmann)
> v1: Initial patch
>
> Fixes: 2ddf71e23cc2 ("net: add notifier hooks for devmap bpf map")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> kernel/bpf/devmap.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 141710b82a6c..191b79948424 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -512,8 +512,7 @@ static int dev_map_notification(struct notifier_block *notifier,
> struct bpf_dtab_netdev *dev, *odev;
>
> dev = READ_ONCE(dtab->netdev_map[i]);
> - if (!dev ||
> - dev->dev->ifindex != netdev->ifindex)
> + if (!dev || netdev != dev->dev)
> continue;
> odev = cmpxchg(&dtab->netdev_map[i], dev, NULL);
> if (dev == odev)
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Al Viro @ 2018-10-25 2:20 UTC (permalink / raw)
To: Joe Perches
Cc: Fengguang Wu, Willy Tarreau, David Miller, wanghaifine, netdev,
LKML, LKP, Philip Li
In-Reply-To: <dda151c160e42aeb07d158d9c7cd4c1a3341ab5f.camel@perches.com>
On Wed, Oct 24, 2018 at 06:16:31PM -0700, Joe Perches wrote:
> On Thu, 2018-10-25 at 09:11 +0800, Fengguang Wu wrote:
> > CC Philip and LKP team.
> > > Please try to make your first patch in drivers/staging
> > > to get familiar with submitting patches to the kernel.
> > > https://kernelnewbies.org/FirstKernelPatch
> > >
> > > Maybe there's utility in creating a filtering and auto-reply
> > > tool for first time patch submitters for all the vger mailing
> > > lists using some combination of previously known submitters
> > > and the 0-day robot to point those first time submitters of
> > > defective patches to kernelnewbies and staging.
> >
> > Yeah good idea. That feature can be broken into 2 parts:
> >
> > - an email script, which could be added to Linux scripts/ dir
> > - maintain records for telling whether someone is first-time patch submitters
>
> Maybe run checkpatch on those first-time submitter patches too.
OK, now I'm certain that you are trolling...
Joe, what really pisses me off is that it's actually at the expense of original
poster (who had nothing to do with the CoCup) *and* an invitation for a certain
variety of kooks. In probably vain hope of heading that off, here's the
summary of what happened _before_ Joe started to stir the shit:
* code in question is, indeed, (slightly) bogus in mainline.
It reads as "reject negative values for length, truncate positive ones to 4",
but in reality it's "treat everything outside of 0..4 as 4". It's not broken
per se, but it's certainly misleading.
* one possible fix would be to drop the "reject negative values"
completely, another - to move checking for negatives before the truncation.
Patch tried to do the latter.
* the author of the patch has moved the check *too* early -
before the value being tested is even obtained. It's obviously wrong - kernel
newbie or not.
* I sincerely doubt that it was an attempt to introduce a backdoor,
albeit one would've been created if that patch went in as-is. Genuine braino
is much more likely, and we'd all done such.
* such brainos can be surprisingly hard to spot in one's code.
It's too obviously wrong, in a sense - you know what you've meant to write
and you keep seeing that instead of what you've actually written. If you
are really unlucky, that might end up with a few days worth of debugging,
with eventual embarrassed "how the fuck have I managed not to notice that
previous twenty times I went over this function today?" Been there,
done that, and so has everyone who'd actually written anything (other
than the worthless screeds, that is).
* one thing the author of that patch could be blamed for (and most
of us have fucked up that way at one point or another) is not testing the
effect of the damn thing. Modification is local, the change of behaviour -
simple and triggering that code is also trivial. Checking that the patch
has done what you expect it to do would be simple and would've caught the
problem.
`
Code of Conduct is garbage, but neither Dave nor the author
of this patch had anything to do with that mess. If you want to make a point,
do so without shit splatter hitting innocent bystanders - people tend to
get very annoyed by that kind of thing, and with a damn good reason.
Sheesh...
^ permalink raw reply
* Re: [PATCH 2/3] net: ethernet: Remove dummy runtime PM callbacks from Renesas drivers
From: Sergei Shtylyov @ 2018-10-24 17:44 UTC (permalink / raw)
To: Jarkko Nikula, linux-pm
Cc: linux-i2c, Wolfram Sang, netdev, David S . Miller,
linux-renesas-soc, linux-usb, Yoshihiro Shimoda
In-Reply-To: <20181024135134.28456-3-jarkko.nikula@linux.intel.com>
Hello!
On 10/24/2018 04:51 PM, Jarkko Nikula wrote:
> Platform drivers don't need dummy runtime PM callbacks that just return
> success in order to have runtime PM happening. This has changed since
> following commits:
>
> 05aa55dddb9e ("PM / Runtime: Lenient generic runtime pm callbacks")
> 543f2503a956 ("PM / platform_bus: Allow runtime PM by default")
> 8b313a38ecff ("PM / Platform: Use generic runtime PM callbacks directly")
Wow, these are old! :-)
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> Only build tested.
Seems long overdue...
[...]
Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Joe Perches @ 2018-10-25 1:16 UTC (permalink / raw)
To: Fengguang Wu
Cc: Willy Tarreau, David Miller, wanghaifine, netdev, LKML, LKP,
Philip Li
In-Reply-To: <20181025011111.bhbstq5wtrwk26b5@wfg-t540p.sh.intel.com>
On Thu, 2018-10-25 at 09:11 +0800, Fengguang Wu wrote:
> CC Philip and LKP team.
> > Please try to make your first patch in drivers/staging
> > to get familiar with submitting patches to the kernel.
> > https://kernelnewbies.org/FirstKernelPatch
> >
> > Maybe there's utility in creating a filtering and auto-reply
> > tool for first time patch submitters for all the vger mailing
> > lists using some combination of previously known submitters
> > and the 0-day robot to point those first time submitters of
> > defective patches to kernelnewbies and staging.
>
> Yeah good idea. That feature can be broken into 2 parts:
>
> - an email script, which could be added to Linux scripts/ dir
> - maintain records for telling whether someone is first-time patch submitters
Maybe run checkpatch on those first-time submitter patches too.
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Fengguang Wu @ 2018-10-25 1:11 UTC (permalink / raw)
To: Joe Perches
Cc: Willy Tarreau, David Miller, wanghaifine, netdev, LKML, LKP,
Philip Li
In-Reply-To: <49ec92564284b5beb0a151bce1d537b51340d0a8.camel@perches.com>
CC Philip and LKP team.
On Wed, Oct 24, 2018 at 04:46:18PM -0700, Joe Perches wrote:
>(adding Fengguang Wu and lkp)
>
>On Wed, 2018-10-24 at 22:48 +0200, Willy Tarreau wrote:
>> When you read this patch from an apparent first-time contributor (no
>> trace in either LKML, netdev or even google), the level of assurance
>> in the commit message is pretty good, showing that he's not at all a
>> beginner, which doesn't match at all the type of error seen in the
>> code, which doesn't even need to be compiled to see that it will emit
>> a warning and not work as advertised.
>
>Which to me is more of an indication of beginner-ness.
>
>> When you factor in the fact that the code opens a big but very discrete
>> vulnerability, I tend to think that in fact we're not facing a newbie
>> at all but someone deliberately trying to inject a subtle backdoor into
>> the kernel and disguise it as a vague bug fix,
>
>That seems unlikely as it introduces a compilation warning.
>
>A real intentional backdoor from a nefarious source would
>likely be completely correct about compilation and use the
>typical commit subject style.
>
>Anyway, who know for certain right now.
>
>I would have suggested David reply with only his second sentence
>and maybe a pointer to kernelnewbies or staging.
>
>Just something like:
>
> nack: 'len' has no value before the get_user() call.
>
> Please try to make your first patch in drivers/staging
> to get familiar with submitting patches to the kernel.
> https://kernelnewbies.org/FirstKernelPatch
>
>Maybe there's utility in creating a filtering and auto-reply
>tool for first time patch submitters for all the vger mailing
>lists using some combination of previously known submitters
>and the 0-day robot to point those first time submitters of
>defective patches to kernelnewbies and staging.
Yeah good idea. That feature can be broken into 2 parts:
- an email script, which could be added to Linux scripts/ dir
- maintain records for telling whether someone is first-time patch submitters
Thanks,
Fengguang
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Al Viro @ 2018-10-25 1:03 UTC (permalink / raw)
To: Joe Perches; +Cc: David Miller, wanghaifine, netdev, LKML
In-Reply-To: <61d94f2a5563db4d6580c8385c3b93c8eeb3669a.camel@perches.com>
On Wed, Oct 24, 2018 at 11:28:11AM -0700, Joe Perches wrote:
> On Wed, 2018-10-24 at 10:10 -0700, David Miller wrote:
> > From: Wang Hai <wanghaifine@gmail.com>
> > Date: Wed, 24 Oct 2018 23:47:29 +0800
> >
> > > To determine whether len is less than zero, it should be put before
> > > the function min_t, because the return value of min_t is not likely
> > > to be less than zero.
> []
> > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
[snip obviously broken patch]
> > You can't be serious?
>
> I'm not personally taken aback by this but
> there is the new Code of
> Conduct to consider.
>
> John McEnroe earned quite a bit of his
> reputation as an 'enfant terrible' via a
> similar statement.
>
> https://www.youtube.com/watch?v=t0hK1wyrrAU
>
> Perhaps a different word choice next time in
> reply to submitters of ill-considered and/or
> defective patches could be useful.
Please tell me we are being Poe'd...
^ permalink raw reply
* Re: [PATCH RFC v3 0/3] Rlimit for module space
From: Edgecombe, Rick P @ 2018-10-25 1:01 UTC (permalink / raw)
To: daniel@iogearbox.net, jeyu@kernel.org, ard.biesheuvel@linaro.org,
mhocko@kernel.org
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
jannh@google.com, keescook@chromium.org, arjan@linux.intel.com,
netdev@vger.kernel.org, tglx@linutronix.de,
linux-mips@linux-mips.org, linux-s390@vger.kernel.org,
x86@kernel.org, kristen@linux.intel.com, Dock, Deneen T,
catalin.marinas@arm.com, mingo@redhat.com, will.deacon@arm.com,
"kernel-hardening@lists.openwall
In-Reply-To: <d7cb6a8c-b7d6-5c82-6721-2b5387da673f@iogearbox.net>
On Wed, 2018-10-24 at 18:04 +0200, Daniel Borkmann wrote:
> [ +Alexei, netdev ]
>
> On 10/24/2018 05:07 PM, Jessica Yu wrote:
> > +++ Ard Biesheuvel [23/10/18 08:54 -0300]:
> > > On 22 October 2018 at 20:06, Edgecombe, Rick P
> > > <rick.p.edgecombe@intel.com> wrote:
>
> [...]
> > > I think it is wrong to conflate the two things. Limiting the number of
> > > BPF allocations and the limiting number of module allocations are two
> > > separate things, and the fact that BPF reuses module_alloc() out of
> > > convenience does not mean a single rlimit for both is appropriate.
> >
> > Hm, I think Ard has a good point. AFAIK, and correct me if I'm wrong,
> > users of module_alloc() i.e. kprobes, ftrace, bpf, seem to use it
> > because it is an easy way to obtain executable kernel memory (and
> > depending on the needs of the architecture, being additionally
> > reachable via relative branches) during runtime. The side effect is
> > that all these users share the "module" memory space, even though this
> > memory region is not exclusively used by modules (well, personally I
> > think it technically should be, because seeing module_alloc() usage
> > outside of the module loader is kind of a misuse of the module API and
> > it's confusing for people who don't know the reason behind its usage
> > outside of the module loader).
> >
> > Right now I'm not sure if it makes sense to impose a blanket limit on
> > all module_alloc() allocations when the real motivation behind the
> > rlimit is related to BPF, i.e., to stop unprivileged users from
> > hogging up all the vmalloc space for modules with JITed BPF filters.
> > So the rlimit has more to do with limiting the memory usage of BPF
> > filters than it has to do with modules themselves.
> >
> > I think Ard's suggestion of having a separate bpf_alloc/free API makes
> > a lot of sense if we want to keep track of bpf-related allocations
> > (and then the rlimit would be enforced for those). Maybe part of the
> > module mapping space could be carved out for bpf filters (e.g. have
> > BPF_VADDR, BPF_VSIZE, etc like how we have it for modules), or
> > continue sharing the region but explicitly define a separate bpf_alloc
> > API, depending on an architecture's needs. What do people think?
>
> Hmm, I think here are several issues mixed up at the same time which is just
> very confusing, imho:
>
> 1) The fact that there are several non-module users of module_alloc()
> as Jessica notes such as kprobes, ftrace, bpf, for example. While all of
> them are not being modules, they all need to alloc some piece of executable
> memory. It's nothing new, this exists for 7 years now since 0a14842f5a3c
> ("net: filter: Just In Time compiler for x86-64") from BPF side; effectively
> that is even /before/ eBPF existed. Having some different API perhaps for all
> these users seems to make sense if the goal is not to interfere with modules
> themselves. It might also help as a benefit to potentially increase that
> memory pool if we're hitting limits at scale which would not be a concern
> for normal kernel modules since there's usually just a very few of them
> needed (unlike dynamically tracing various kernel parts 24/7 w/ or w/o BPF,
> running BPF-seccomp policies, networking BPF policies, etc which need to
> scale w/ application or container deployment; so this is of much more
> dynamic and unpredictable nature).
>
> 2) Then there is rlimit which is proposing to have a "fairer" share among
> unprivileged users. I'm not fully sure yet whether rlimit is actually a
> nice usable interface for all this. I'd agree that something is needed
> on that regard, but I also tend to like Michal Hocko's cgroup proposal,
> iow, to have such resource control as part of memory cgroup could be
> something to consider _especially_ since it already _exists_ for vmalloc()
> backed memory so this should be not much different than that. It sounds
> like 2) comes on top of 1).
FWIW, cgroups seems like a better solution than rlimit to me too. Maybe you all
already know, but it looks like the cgroups vmalloc charge is done in the main
page allocator and counts against the whole kernel memory limit. A user may want
to have a higher kernel limit than the module space size, so it seems it isn't
enough by itself and some new limit would need to be added.
As for what the limit should be, I wonder if some of the disagreement is just
from the name "module space".
There is a limited resource of physical memory, so we have limits for it. There
is a limited resource of CPU time, so we have limits for it. If there is a
limited resource for preferred address space for executable code, why not just
continue that trend? If other forms of unprivileged JIT come along, would it be
better to have N limits for each type? Request_module probably can't fill the
space, but technically there are already 2 unprivileged users. So IMHO, its a
more forward looking solution.
If there are some usage/architecture combos that don't need the preferred space
they can allocate in vmalloc and have it not count against the preferred space
limit but still count against the cgroups kernel memory limit.
Another benefit of centralizing the allocation of the "executable memory
preferred space" is KASLR. Right now that is only done in module_alloc and so
all users of it get randomized. If they all call vmalloc by themselves they will
just use the normal allocator.
Anyway, it seems like either type of limit (BPF JIT or all module space) will
solve the problem equally well today.
> 3) Last but not least, there's a short term fix which is needed independently
> of 1) and 2) and should be done immediately which is to account for
> unprivileged users and restrict them based on a global configurable
> limit such that privileged use keeps functioning, and 2) could enforce
> based on the global upper limit, for example. Pending fix is under
> https://patchwork.ozlabs.org/patch/987971/ which we intend to ship to
> Linus as soon as possible as short term fix. Then something like memcg
> can be considered on top since it seems this makes most sense from a
> usability point.
>
> Thanks a lot,
> Daniel
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Eric Dumazet @ 2018-10-25 0:50 UTC (permalink / raw)
To: Joe Perches, David Miller
Cc: w, wanghaifine, netdev, linux-kernel, fengguang.wu, lkp
In-Reply-To: <825268591809f66eb475c3b41c327809a304388f.camel@perches.com>
On 10/24/2018 05:23 PM, Joe Perches wrote:
>
> "You can't be serious?" is kind?
Context maybe ? As a non native, I do not see why it is an offense.
I would like very much we discuss about patches here, not about whatever
interpretation you or anyone could make from any answers.
Recipe for disaster in linux community :
1) Write a bot, sending one wrong patch every hour, with a random "From:" name
and email address.
Yeah, can you believe a bot can actually 'Signed-off-by:' a patch ???
2) Hundred of emails sent, from reviewers, annoyed maintainers, and flames
because the proper words for newbies were not _carefully_ chosen.
So just wait for an answer from the supposed patch author, and see what happens next.
Thank you.
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Richard Guy Briggs @ 2018-10-25 0:42 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhRF1vsxM-k0Lw-9NqS9b9rgXRRBu0tq4ajdFpMqUxiH4A@mail.gmail.com>
On 2018-10-24 16:55, Paul Moore wrote:
> On Wed, Oct 24, 2018 at 11:15 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-10-19 19:16, Paul Moore wrote:
> > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Create a new audit record AUDIT_CONTAINER to document the audit
> > > > container identifier of a process if it is present.
> > > >
> > > > Called from audit_log_exit(), syscalls are covered.
> > > >
> > > > A sample raw event:
> > > > type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> > > > type=CWD msg=audit(1519924845.499:257): cwd="/root"
> > > > type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > > > type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > > > type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> > > > type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > > See: https://github.com/linux-audit/audit-userspace/issues/51
> > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > Acked-by: Serge Hallyn <serge@hallyn.com>
> > > > Acked-by: Steve Grubb <sgrubb@redhat.com>
> > > > ---
> > > > include/linux/audit.h | 7 +++++++
> > > > include/uapi/linux/audit.h | 1 +
> > > > kernel/audit.c | 24 ++++++++++++++++++++++++
> > > > kernel/auditsc.c | 3 +++
> > > > 4 files changed, 35 insertions(+)
> > >
> > > ...
> > >
> > > > @@ -2045,6 +2045,30 @@ void audit_log_session_info(struct audit_buffer *ab)
> > > > audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> > > > }
> > > >
> > > > +/*
> > > > + * audit_log_contid - report container info
> > > > + * @tsk: task to be recorded
> > > > + * @context: task or local context for record
> > > > + * @op: contid string description
> > > > + */
> > > > +int audit_log_contid(struct task_struct *tsk,
> > > > + struct audit_context *context, char *op)
> > > > +{
> > > > + struct audit_buffer *ab;
> > > > +
> > > > + if (!audit_contid_set(tsk))
> > > > + return 0;
> > > > + /* Generate AUDIT_CONTAINER record with container ID */
> > > > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> > > > + if (!ab)
> > > > + return -ENOMEM;
> > > > + audit_log_format(ab, "op=%s contid=%llu",
> > > > + op, audit_get_contid(tsk));
> > > > + audit_log_end(ab);
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(audit_log_contid);
> > >
> > > As discussed in the previous iteration of the patch, I prefer
> > > AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If you feel strongly
> > > about keeping it as-is with AUDIT_CONTAINER I suppose I could live
> > > with that, but it is isn't my first choice.
> >
> > I don't have a strong opinion on this one, mildly preferring the shorter
> > one only because it is shorter.
>
> We already have multiple AUDIT_CONTAINER* record types, so it seems as
> though we should use "AUDIT_CONTAINER" as a prefix of sorts, rather
> than a type itself.
I'm fine with that. I'd still like to hear Steve's input. He had
stronger opinions than me.
> > > However, I do care about the "op" field in this record. It just
> > > doesn't make any sense; the way you are using it it is more of a
> > > context field than an operations field, and even then why is the
> > > context important from a logging and/or security perspective? Drop it
> > > please.
> >
> > I'll rename it to whatever you like. I'd suggest "ref=". The reason I
> > think it is important is there are multiple sources that aren't always
> > obvious from the other records to which it is associated. In the case
> > of ptrace and signals, there can be many target tasks listed (OBJ_PID)
> > with no other way to distinguish the matching audit container identifier
> > records all for one event. This is in addition to the default syscall
> > container identifier record. I'm not currently happy with the text
> > content to link the two, but that should be solvable (most obvious is
> > taret PID). Throwing away this information seems shortsighted.
>
> It would be helpful if you could generate real audit events
> demonstrating the problems you are describing, as well as a more
> standard syscall event, so we can discuss some possible solutions.
If the auditted process is in a container and it ptraces or signals
another process in a container, there will be two AUDIT_CONTAINER
records for the same event that won't be identified as to which record
belongs to which process or other record (SYSCALL vs 1+ OBJ_PID
records). There could be many signals recorded, each with their own
OBJ_PID record. The first is stored in the audit context and additional
ones are stored in a chained struct that can accommodate 16 entries each.
(See audit_signal_info(), __audit_ptrace().)
(As a side note, on code inspection it appears that a signal target
would get overwritten by a ptrace action if they were to happen in that
order.)
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Joe Perches @ 2018-10-25 0:23 UTC (permalink / raw)
To: David Miller; +Cc: w, wanghaifine, netdev, linux-kernel, fengguang.wu, lkp
In-Reply-To: <20181024.170213.632063387857625082.davem@davemloft.net>
On Wed, 2018-10-24 at 17:02 -0700, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Wed, 24 Oct 2018 16:46:18 -0700
>
> > I would have suggested David reply with only his second sentence
> > and maybe a pointer to kernelnewbies or staging.is
>
> I maintain that I was not out of line with my comment.
>
> I sought a second opinion from Greg KH and others, and they agree with
> me that I was still kind with my choice of words.
"You can't be serious?" is kind?
> I think you're taking things too far, and I will simply not stand for
> this overreaching judgement upon my behavior on this list which is
> completely not justified.
Even above here, concision is generally better.
overreaching and completely are probably not reasonable.
openness and defensiveness are somewhat antithetic.
>From the new Code of Conduct (which I don't even approve)
* Gracefully accepting constructive criticism.
cheers, Joe
^ permalink raw reply
* [PATCH 2/2] Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
From: Matthias Kaehlcke @ 2018-10-25 0:21 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, David S . Miller, Loic Poulain
Cc: linux-bluetooth, netdev, linux-kernel, Balakrishna Godavarthi,
Brian Norris, Dmitry Grinberg, Matthias Kaehlcke
In-Reply-To: <20181025002134.256791-1-mka@chromium.org>
Use the HCI_QUIRK_USE_BDADDR_PROPERTY quirk to let the HCI
core handle the reading of 'local-bd-address'. With this there
is no need to set HCI_QUIRK_INVALID_BDADDR, the case of a
non-existing or invalid fwnode property is handled by the core
code.
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
I couldn't actually test the changes in this driver since I
don't have a device with this controller. Could someone
from Qualcomm help with this?
---
drivers/bluetooth/btqcomsmd.c | 28 +++-------------------------
1 file changed, 3 insertions(+), 25 deletions(-)
diff --git a/drivers/bluetooth/btqcomsmd.c b/drivers/bluetooth/btqcomsmd.c
index 7df3eed1ef5e..e5841602c4f1 100644
--- a/drivers/bluetooth/btqcomsmd.c
+++ b/drivers/bluetooth/btqcomsmd.c
@@ -125,23 +125,10 @@ static int btqcomsmd_setup(struct hci_dev *hdev)
return PTR_ERR(skb);
kfree_skb(skb);
- /* Devices do not have persistent storage for BD address. If no
- * BD address has been retrieved during probe, mark the device
- * as having an invalid BD address.
+ /* Devices do not have persistent storage for BD address. Retrieve
+ * it from the firmware node property.
*/
- if (!bacmp(&btq->bdaddr, BDADDR_ANY)) {
- set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
- return 0;
- }
-
- /* When setting a configured BD address fails, mark the device
- * as having an invalid BD address.
- */
- err = qca_set_bdaddr_rome(hdev, &btq->bdaddr);
- if (err) {
- set_bit(HCI_QUIRK_INVALID_BDADDR, &hdev->quirks);
- return 0;
- }
+ set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
return 0;
}
@@ -169,15 +156,6 @@ static int btqcomsmd_probe(struct platform_device *pdev)
if (IS_ERR(btq->cmd_channel))
return PTR_ERR(btq->cmd_channel);
- /* The local-bd-address property is usually injected by the
- * bootloader which has access to the allocated BD address.
- */
- if (!of_property_read_u8_array(pdev->dev.of_node, "local-bd-address",
- (u8 *)&btq->bdaddr, sizeof(bdaddr_t))) {
- dev_info(&pdev->dev, "BD address %pMR retrieved from device-tree",
- &btq->bdaddr);
- }
-
hdev = hci_alloc_dev();
if (!hdev)
return -ENOMEM;
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related
* [PATCH 0/2] Add quirk for reading BD_ADDR from fwnode property
From: Matthias Kaehlcke @ 2018-10-25 0:21 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, David S . Miller, Loic Poulain
Cc: linux-bluetooth, netdev, linux-kernel, Balakrishna Godavarthi,
Brian Norris, Dmitry Grinberg, Matthias Kaehlcke
On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
on the Bluetooth chip itself. One way to configure the address is
through the device tree (patched in by the bootloader). The btqcomsmd
driver is an example, it can read the address from the DT property
'local-bd-address'. It is also planned to extend the hci_qca driver
to support setting the BD address through the DT.
To avoid redundant open-coded reading of 'local-bd-address' and error
handling this series adds the quirk HCI_QUIRK_USE_BDADDR_PROPERTY to
retrieve the BD address of a device from the DT and adapts the
btqcomsmd driver to use this quirk.
Matthias Kaehlcke (2):
Bluetooth: Add quirk for reading BD_ADDR from fwnode property
Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
drivers/bluetooth/btqcomsmd.c | 28 +++--------------------
include/net/bluetooth/hci.h | 12 ++++++++++
net/bluetooth/hci_core.c | 42 +++++++++++++++++++++++++++++++++++
net/bluetooth/mgmt.c | 6 +++--
4 files changed, 61 insertions(+), 27 deletions(-)
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply
* [PATCH] Change judgment len position
From: Wang Hai @ 2018-10-24 15:47 UTC (permalink / raw)
To: edumazet; +Cc: davem, kuznet, yoshfuji, netdev, linux-kernel, Wang Hai
To determine whether len is less than zero, it should be put before
the function min_t, because the return value of min_t is not likely
to be less than zero.
Signed-off-by: Wang Hai <wanghaifine@gmail.com>
---
net/ipv4/tcp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 10c624639..49af9fdc3 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3301,11 +3301,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
struct net *net = sock_net(sk);
int val, len;
+ len = min_t(unsigned int, len, sizeof(int));
+
if (get_user(len, optlen))
return -EFAULT;
- len = min_t(unsigned int, len, sizeof(int));
-
if (len < 0)
return -EINVAL;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] r8169: Add new device ID support
From: Stephen Hemminger @ 2018-10-24 15:47 UTC (permalink / raw)
To: Shawn Lin; +Cc: David Miller, nic_swsd, netdev
In-Reply-To: <db4ae0f0-97e4-d2c1-9ac9-e689e3d80447@rock-chips.com>
On Wed, 24 Oct 2018 14:29:34 +0800
Shawn Lin <shawn.lin@rock-chips.com> wrote:
> On 2018/10/24 13:54, David Miller wrote:
> > From: Shawn Lin <shawn.lin@rock-chips.com>
> > Date: Wed, 24 Oct 2018 13:48:55 +0800
> >
> >> Hi David,
> >>
> >> On 2018/10/24 10:19, David Miller wrote:
> >>> From: Shawn Lin <shawn.lin@rock-chips.com>
> >>> Date: Wed, 24 Oct 2018 09:46:47 +0800
> >>>
> >>>> It's found my r8169 ethernet card at hand has a device ID
> >>>> of 0x0000 which wasn't on the list of rtl8169_pci_tbl. Add
> >>>> a new entry to make it work:
> >>> ...
> >>>> 01:00.0 Class 0200: 10ec:0000
> >>> I don't know about this.
> >>> A value of zero could mean the device is mis-responding to
> >>> PCI config space requests or something like that.
> >>
> >> It was working fine on my retired Windows XP home PC with same devcice
> >> ID listed, so I guess r8169 driver for windows system knows 0x0000 is
> >> also valid.
> >
> > It is also possible the device comes up in a different state.
> >
> > Under windows does it show with that device ID of zero?
>
> yup. More precisely, I checked how BIOS enumerate it by PCIe analyzer
> and see it does report 0x0000 as device ID.
>
> >
> >
> >
>
Look at device manager properties of the device in Windows?
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: David Miller @ 2018-10-25 0:02 UTC (permalink / raw)
To: joe; +Cc: w, wanghaifine, netdev, linux-kernel, fengguang.wu, lkp
In-Reply-To: <49ec92564284b5beb0a151bce1d537b51340d0a8.camel@perches.com>
From: Joe Perches <joe@perches.com>
Date: Wed, 24 Oct 2018 16:46:18 -0700
> I would have suggested David reply with only his second sentence
> and maybe a pointer to kernelnewbies or staging.
I maintain that I was not out of line with my comment.
I sought a second opinion from Greg KH and others, and they agree with
me that I was still kind with my choice of words.
I think you're taking things too far, and I will simply not stand for
this overreaching judgement upon my behavior on this list which is
completely not justified.
Thank you.
^ permalink raw reply
* [PATCH net] net: sched: Remove TCA_OPTIONS from policy
From: David Ahern @ 2018-10-24 15:32 UTC (permalink / raw)
To: netdev, davem, pupilla; +Cc: David Ahern
From: David Ahern <dsahern@gmail.com>
Marco reported an error with hfsc:
root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
Error: Attribute failed policy validation.
Apparently a few implementations pass TCA_OPTIONS as a binary instead
of nested attribute, so drop TCA_OPTIONS from the policy.
Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
Reported-by: Marco Berizzi <pupilla@libero.it>
Signed-off-by: David Ahern <dsahern@gmail.com>
---
net/sched/sch_api.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 3dc0acf54245..be7cd140b2a3 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1309,7 +1309,6 @@ check_loop_fn(struct Qdisc *q, unsigned long cl, struct qdisc_walker *w)
const struct nla_policy rtm_tca_policy[TCA_MAX + 1] = {
[TCA_KIND] = { .type = NLA_STRING },
- [TCA_OPTIONS] = { .type = NLA_NESTED },
[TCA_RATE] = { .type = NLA_BINARY,
.len = sizeof(struct tc_estimator) },
[TCA_STAB] = { .type = NLA_NESTED },
--
2.11.0
^ permalink raw reply related
* Re: [net-next][PATCH] net/ipv4: fix a net leak
From: David Ahern @ 2018-10-24 15:21 UTC (permalink / raw)
To: David Ahern, Li RongQing, netdev, David Miller
In-Reply-To: <92af7409-6154-eb85-7d2f-56f5288b8894@gmail.com>
On 10/24/18 9:02 AM, David Ahern wrote:
> On 10/24/18 3:36 AM, Li RongQing wrote:
>> put net when input a invalid ifindex, otherwise it will be leaked
>>
>> Fixes: 5fcd266a9f64("net/ipv4: Add support for dumping addresses for a specific device")
>> Cc: David Ahern <dsahern@gmail.com>
>> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>> ---
>> net/ipv4/devinet.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
>> index 63d5b58fbfdb..fd0c5a47e742 100644
>> --- a/net/ipv4/devinet.c
>> +++ b/net/ipv4/devinet.c
>> @@ -1775,8 +1775,10 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>>
>> if (fillargs.ifindex) {
>> dev = __dev_get_by_index(tgt_net, fillargs.ifindex);
>> - if (!dev)
>> + if (!dev) {
>> + put_net(tgt_net);
>> return -ENODEV;
>> + }
>>
>> in_dev = __in_dev_get_rtnl(dev);
>> if (in_dev) {
>>
>
> Good catch. IPv6 has the same problem. Will fix that one.
>
Actually remove that 'Reviewed-by'. You should only call put_net if
(fillargs.netnsid >= 0)
DaveM: just want to call this out since I mistakenly added the
Reviewed-by. This patch should be dropped.
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Joe Perches @ 2018-10-24 23:46 UTC (permalink / raw)
To: Willy Tarreau; +Cc: David Miller, wanghaifine, netdev, LKML, Fengguang Wu, lkp
In-Reply-To: <20181024204852.GA25509@1wt.eu>
(adding Fengguang Wu and lkp)
On Wed, 2018-10-24 at 22:48 +0200, Willy Tarreau wrote:
> When you read this patch from an apparent first-time contributor (no
> trace in either LKML, netdev or even google), the level of assurance
> in the commit message is pretty good, showing that he's not at all a
> beginner, which doesn't match at all the type of error seen in the
> code, which doesn't even need to be compiled to see that it will emit
> a warning and not work as advertised.
Which to me is more of an indication of beginner-ness.
> When you factor in the fact that the code opens a big but very discrete
> vulnerability, I tend to think that in fact we're not facing a newbie
> at all but someone deliberately trying to inject a subtle backdoor into
> the kernel and disguise it as a vague bug fix,
That seems unlikely as it introduces a compilation warning.
A real intentional backdoor from a nefarious source would
likely be completely correct about compilation and use the
typical commit subject style.
Anyway, who know for certain right now.
I would have suggested David reply with only his second sentence
and maybe a pointer to kernelnewbies or staging.
Just something like:
nack: 'len' has no value before the get_user() call.
Please try to make your first patch in drivers/staging
to get familiar with submitting patches to the kernel.
https://kernelnewbies.org/FirstKernelPatch
Maybe there's utility in creating a filtering and auto-reply
tool for first time patch submitters for all the vger mailing
lists using some combination of previously known submitters
and the 0-day robot to point those first time submitters of
defective patches to kernelnewbies and staging.
cheers, Joe
^ permalink raw reply
* Re: [net-next][PATCH] net/ipv4: fix a net leak
From: David Ahern @ 2018-10-24 15:02 UTC (permalink / raw)
To: Li RongQing, netdev
In-Reply-To: <1540373788-15078-1-git-send-email-lirongqing@baidu.com>
On 10/24/18 3:36 AM, Li RongQing wrote:
> put net when input a invalid ifindex, otherwise it will be leaked
>
> Fixes: 5fcd266a9f64("net/ipv4: Add support for dumping addresses for a specific device")
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> net/ipv4/devinet.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index 63d5b58fbfdb..fd0c5a47e742 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1775,8 +1775,10 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
>
> if (fillargs.ifindex) {
> dev = __dev_get_by_index(tgt_net, fillargs.ifindex);
> - if (!dev)
> + if (!dev) {
> + put_net(tgt_net);
> return -ENODEV;
> + }
>
> in_dev = __in_dev_get_rtnl(dev);
> if (in_dev) {
>
Good catch. IPv6 has the same problem. Will fix that one.
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: Attribute failed policy validation
From: David Ahern @ 2018-10-24 14:58 UTC (permalink / raw)
To: Florian Westphal, Marco Berizzi; +Cc: netdev, dsahern
In-Reply-To: <20181024130742.777kcbzp667mo5rm@breakpoint.cc>
On 10/24/18 7:07 AM, Florian Westphal wrote:
> Marco Berizzi <pupilla@libero.it> wrote:
>
> [ CC David ]
>
>> root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
>> Error: Attribute failed policy validation.
>
> caused by:
> commit 8b4c3cdd9dd8290343ce959a132d3b334062c5b9
> net: sched: Add policy validation for tc attributes
>
> Looks like TCA_OPTIONS is NLA_BINARY in hfsc case, so
> we can't enforce NLA_NESTED :-(
>
Yep. I'll send a fix.
^ permalink raw reply
* Re: [PATCH 0/3] PM: Renesas: Remove dummy runtime PM callbacks
From: Wolfram Sang @ 2018-10-24 14:22 UTC (permalink / raw)
To: Jarkko Nikula, Magnus Damm
Cc: linux-pm, linux-i2c, Wolfram Sang, netdev, David S . Miller,
Sergei Shtylyov, linux-renesas-soc, linux-usb, Yoshihiro Shimoda
In-Reply-To: <20181024135134.28456-1-jarkko.nikula@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]
On Wed, Oct 24, 2018 at 04:51:31PM +0300, Jarkko Nikula wrote:
> I noticed these independent Renesas drivers have needless dummy runtime
> PM callbacks. I don't have the HW so only build tested.
>
> Patches can be applied independently to their own subsystems. I wanted to
> send them together if some of them gets Tested-by or sees a regression.
At least the I2C driver part of this was on my todo list as well (just a
bit lower :/). I wanted to find out why they have been there in the
first place. Do you know if such callbacks were needed "back in the
days"?
Adding Magnus to recipients...
>
> Jarkko Nikula (3):
> i2c: sh_mobile: Remove dummy runtime PM callbacks
> net: ethernet: Remove dummy runtime PM callbacks from Renesas drivers
> usb: renesas_usbhs: Remove dummy runtime PM callbacks
>
> drivers/i2c/busses/i2c-sh_mobile.c | 18 ------------------
> drivers/net/ethernet/renesas/ravb_main.c | 13 -------------
> drivers/net/ethernet/renesas/sh_eth.c | 13 -------------
> drivers/usb/renesas_usbhs/common.c | 14 --------------
> 4 files changed, 58 deletions(-)
>
> --
> 2.19.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/7] include: add setbits_leXX/clrbits_leXX/clrsetbits_leXX in linux/setbits.h
From: Jakub Kicinski @ 2018-10-24 22:46 UTC (permalink / raw)
To: Corentin Labbe
Cc: Gilles.Muller, Julia.Lawall, agust, airlied, alexandre.torgue,
alistair, benh, carlo, davem, galak, joabreu, khilman,
matthias.bgg, maxime.ripard, michal.lkml, mpe, mporter,
narmstrong, nicolas.palix, oss, paulus, peppe.cavallaro, tj, vitb,
wens, cocci, dri-devel, linux-amlogic, linux-arm-kernel,
linux-ide, linux-kernel, linux-mediatek, linuxppc-dev
In-Reply-To: <1540366553-18541-3-git-send-email-clabbe@baylibre.com>
On Wed, 24 Oct 2018 07:35:48 +0000, Corentin Labbe wrote:
> This patch adds setbits_le32/clrbits_le32/clrsetbits_le32 and
> setbits_le64/clrbits_le64/clrsetbits_le64 in linux/setbits.h header.
>
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
Did you have a look at all the functions defined in bitfield.h?
(including the magic macro-generated ones?) Perhaps those could be
used here?
I also share the concerns about the non-atomic RMW. Obviously correct
beats short IMHO.
> diff --git a/include/linux/setbits.h b/include/linux/setbits.h
> new file mode 100644
> index 000000000000..c82faf8d7fe4
> --- /dev/null
> +++ b/include/linux/setbits.h
> @@ -0,0 +1,84 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_SETBITS_H
> +#define __LINUX_SETBITS_H
> +
> +#include <linux/io.h>
> +
> +#define __setbits(rfn, wfn, addr, set) wfn((rfn(addr) | (set)), addr)
> +#define __clrbits(rfn, wfn, addr, mask) wfn((rfn(addr) & ~(mask)), addr)
> +#define __clrsetbits(rfn, wfn, addr, mask, set) wfn(((rfn(addr) & ~(mask)) | (set)), addr)
> +#define __setclrbits(rfn, wfn, addr, mask, set) wfn(((rfn(addr) | (set)) & ~(mask)), addr)
> +
> +#ifndef setbits_le32
> +#define setbits_le32(addr, set) __setbits(readl, writel, addr, set)
> +#endif
> +#ifndef setbits_le32_relaxed
> +#define setbits_le32_relaxed(addr, set) __setbits(readl_relaxed, writel_relaxed, \
> + addr, set)
> +#endif
> +
> +#ifndef clrbits_le32
> +#define clrbits_le32(addr, mask) __clrbits(readl, writel, addr, mask)
> +#endif
> +#ifndef clrbits_le32_relaxed
> +#define clrbits_le32_relaxed(addr, mask) __clrbits(readl_relaxed, writel_relaxed, \
> + addr, mask)
> +#endif
> +
> +#ifndef clrsetbits_le32
> +#define clrsetbits_le32(addr, mask, set) __clrsetbits(readl, writel, addr, mask, set)
> +#endif
> +#ifndef clrsetbits_le32_relaxed
> +#define clrsetbits_le32_relaxed(addr, mask, set) __clrsetbits(readl_relaxed, \
> + writel_relaxed, \
> + addr, mask, set)
> +#endif
> +
> +#ifndef setclrbits_le32
> +#define setclrbits_le32(addr, mask, set) __setclrbits(readl, writel, addr, mask, set)
> +#endif
> +#ifndef setclrbits_le32_relaxed
> +#define setclrbits_le32_relaxed(addr, mask, set) __setclrbits(readl_relaxed, \
> + writel_relaxed, \
> + addr, mask, set)
> +#endif
> +
> +/* We cannot use CONFIG_64BIT as some x86 drivers use non-atomicwriteq() */
> +#if defined(writeq) && defined(readq)
> +#ifndef setbits_le64
> +#define setbits_le64(addr, set) __setbits(readq, writeq, addr, set)
> +#endif
> +#ifndef setbits_le64_relaxed
> +#define setbits_le64_relaxed(addr, set) __setbits(readq_relaxed, writeq_relaxed, \
> + addr, set)
> +#endif
> +
> +#ifndef clrbits_le64
> +#define clrbits_le64(addr, mask) __clrbits(readq, writeq, addr, mask)
> +#endif
> +#ifndef clrbits_le64_relaxed
> +#define clrbits_le64_relaxed(addr, mask) __clrbits(readq_relaxed, writeq_relaxed, \
> + addr, mask)
> +#endif
> +
> +#ifndef clrsetbits_le64
> +#define clrsetbits_le64(addr, mask, set) __clrsetbits(readq, writeq, addr, mask, set)
> +#endif
> +#ifndef clrsetbits_le64_relaxed
> +#define clrsetbits_le64_relaxed(addr, mask, set) __clrsetbits(readq_relaxed, \
> + writeq_relaxed, \
> + addr, mask, set)
> +#endif
> +
> +#ifndef setclrbits_le64
> +#define setclrbits_le64(addr, mask, set) __setclrbits(readq, writeq, addr, mask, set)
> +#endif
> +#ifndef setclrbits_le64_relaxed
> +#define setclrbits_le64_relaxed(addr, mask, set) __setclrbits(readq_relaxed, \
> + writeq_relaxed, \
> + addr, mask, set)
> +#endif
> +
> +#endif /* writeq/readq */
> +
> +#endif /* __LINUX_SETBITS_H */
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox