Re: [PATCH v2] netlink: Replace rhash_portid with bound

Hello, David. On Thu, Sep 24, 2015 at 12:11:42PM -0700, David Miller wrote: > From: Herbert Xu > Date: Tue, 22 Sep 2015 11:38:56 +0800 > > > The commit 1f770c0a09da855a2b51af6d19de97fb955eca85 ("netlink: > > Fix autobind race condition that leads to zero port ID")

Re: [PATCH v2] netlink: Replace rhash_portid with bound

From: Herbert Xu Date: Tue, 22 Sep 2015 11:38:56 +0800 > The commit 1f770c0a09da855a2b51af6d19de97fb955eca85 ("netlink: > Fix autobind race condition that leads to zero port ID") created > some new races that can occur due to inconcsistencies between the > two port

Re: [PATCH v2] netlink: Replace rhash_portid with bound

Hello, Herbert. On Wed, Sep 23, 2015 at 02:13:42PM +0800, Herbert Xu wrote: > It doesn't matter on x86 but the semantics of smp_load_acquire > and smp_store_release explicitly includes a full barrier which > is something that we don't need. Yeah, I was confused there. I was thinking smp_rmb()

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Tue, Sep 22, 2015 at 12:10:56PM -0400, Tejun Heo wrote: > > That's a pentium pro era errata. Virtually no working machine is > affected by that anymore and nobody builds kernel with that option. > In most cases, store_release and load_acquire are cheaper as they're > more specific. On x86,

Re: [PATCH v2] netlink: Replace rhash_portid with bound

Hello, On Thu, Sep 24, 2015 at 11:21:00AM +0800, Herbert Xu wrote: > Well we'll have to agree to disagree on that one. I have seen too > many instances over the years where people post patches that use > primitives such as RCU and think that they must be safe because > it compiles with no

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Wed, Sep 23, 2015 at 11:29:28PM -0400, Tejun Heo wrote: > > So, while that also has been a common failure mode that we've been > seeing with barrier usages, what you're suggesting isn't the right > balance either. It's error-prone in a different way as amply > exemplified in this very thread.

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Wed, Sep 23, 2015 at 11:41:16PM -0400, Tejun Heo wrote: > On Thu, Sep 24, 2015 at 11:31:17AM +0800, Herbert Xu wrote: > > No this isn't what happened. My error was trying to see if there > > is a way to do it without barriers. In the end there wasn't. This > > has nothing to do with using

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Thu, Sep 24, 2015 at 11:31:17AM +0800, Herbert Xu wrote: > No this isn't what happened. My error was trying to see if there > is a way to do it without barriers. In the end there wasn't. This > has nothing to do with using primitives. Hmmm... yeah, you can say that, but it still was a

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Wed, Sep 23, 2015 at 11:43:21PM -0400, Tejun Heo wrote: > On Thu, Sep 24, 2015 at 11:42:14AM +0800, Herbert Xu wrote: > > Well I disagree so let's leave it at that. > > Leaving things disagreed is fine but there's still a patch to commit > here, so I get that you're still dead against just

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Thu, Sep 24, 2015 at 11:42:14AM +0800, Herbert Xu wrote: > Well I disagree so let's leave it at that. Leaving things disagreed is fine but there's still a patch to commit here, so I get that you're still dead against just applying the pattern? -- tejun -- To unsubscribe from this list: send

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Wed, Sep 23, 2015 at 11:54:40AM -0400, Tejun Heo wrote: > > Hmm... lemme try again. When using barriers or RCU, it's desirable to > establish certain invariants because it usually is extremely easy to > miss corner cases. It is helpful to have an abstraction, even if just > conceptual, where

Re: [PATCH v2] netlink: Replace rhash_portid with bound

Hello, Herbert. On Thu, Sep 24, 2015 at 10:30:11AM +0800, Herbert Xu wrote: > Well if someone provided helpers which > > 1) uses smp_wmb and smp_rmb instead of full barriers; This part is fine. > 2) provides raw variants for the cases that barriers aren't needed Hmm... It looks like I'm

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Wed, Sep 23, 2015 at 10:46:08PM -0400, Tejun Heo wrote: > > Hmm... It looks like I'm failing at communicating. Lemme try again. > There are two situations where we do this. > > 1. When there are different locking contexts. In this case, the write >path is. It's already protected by the

Re: [PATCH v2] netlink: Replace rhash_portid with bound

Hello, On Thu, Sep 24, 2015 at 10:54:36AM +0800, Herbert Xu wrote: > What I am concerned about is the next guy who comes along and > does a rewrite like the one that introduced the netlink_bind > bug. That person needs to fully understand what each primitive > is protecting against. > > Using

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Wed, Sep 23, 2015 at 11:06:09PM -0400, Tejun Heo wrote: > > I think this is where we're not agreeing. My point is that better > understanding and lower likelihood of bug doesn't equate specializing > each usage site. That's a lot more likely to lead to unnecessary > cognition overhead and

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Tue, Sep 22, 2015 at 9:10 AM, Tejun Heo wrote: > > That's a pentium pro era errata. Virtually no working machine is > affected by that anymore and nobody builds kernel with that option. > In most cases, store_release and load_acquire are cheaper as they're > more specific.

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Tue, Sep 22, 2015 at 11:53 AM, Tejun Heo wrote: > > I see. The write path here is cold so the competition is between rmb > and acquire. Unless some significant archs completely screwed it up, > acquire still seems like the better option. It's essentially free on > x86 after

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Tue, Sep 22, 2015 at 12:50 PM, Tejun Heo wrote: > > Hmmm? We're doing that. PPRO_FENCE makes both acquire and release > use smp_mb(). Oh, right you are. I just grepped for rmb, and missed it because as you say, it's a full mb. The PPRO_FENCE_BUG thing is rather

Re: [PATCH v2] netlink: Replace rhash_portid with bound

Hello, On Tue, Sep 22, 2015 at 12:28:45PM -0700, Linus Torvalds wrote: > Both acquire and smp_rmb() are equally free on x86. Was this always like this? Ah, okay, from 2007. Was thinking it's still doing an actual rmb. Talk about being confused. > It appears that we don't do the

Re: [PATCH v2] netlink: Replace rhash_portid with bound

Hello, Linus. On Tue, Sep 22, 2015 at 11:42:33AM -0700, Linus Torvalds wrote: ... > smp_rmb() should generally be about the same cost as an acquire. It > can go either way. > > So *if* the algorithm is amenable to smp_wmb()/smp_rmb() kind of > barriers, that's actually quite possibly better than

Re: [PATCH v2] netlink: Replace rhash_portid with bound

Linus Torvalds writes: > The PPRO_FENCE_BUG thing is rather questionable. I'm not sure it's > even documented, but I can't find the official PPro errata now. I > think I discussed it with Andy Glew back when Intel was starting to > document the memory ordering

Re: [PATCH v2] netlink: Replace rhash_portid with bound

On Tue, Sep 22, 2015 at 1:36 PM, Bjørn Mork wrote: > > http://download.intel.com/design/archives/processors/pro/docs/24268935.pdf > > Says "NoFix" for erratas 66 and 92. Yeah, 66 and 92 do look like they could cause the apparent ordering of accesses to be violated. That said, both