lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 3 Aug 2022 09:33:48 +0000
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     "kuba@...nel.org" <kuba@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        Tariq Toukan <tariqt@...dia.com>,
        Gal Pressman <gal@...dia.com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        Boris Pismenny <borisp@...dia.com>
Subject: Re: [PATCH net-next] net/tls: Use RCU API to access tls_ctx->netdev

On Tue, 2022-08-02 at 08:37 -0700, Jakub Kicinski wrote:
> On Tue, 2 Aug 2022 12:03:32 +0000 Maxim Mikityanskiy wrote:
> > > For cases like this where we don't actually hold onto the object, just
> > > take a peek at the address of it we can save a handful of LoC by using
> > > rcu_access_pointer().   
> > 
> > The documentation of rcu_access_pointer says it shouldn't be used on
> > the update side, because we lose lockdep protection:
> > 
> > --cut--
> > 
> > Although rcu_access_pointer() may also be used in cases
> > where update-side locks prevent the value of the pointer from changing,
> > you should instead use rcu_dereference_protected() for this use case.
> 
> I think what this is trying to say is to not use the
> rcu_access_pointer() as a hack against lockdep:

Well, maybe we understand it in different ways. This is how I parsed it
(the whole comment):

1. rcu_access_pointer is not for the read side. So, it's either for the
write side or for usage outside all locks.

2. It's not for dereferencing. So, it's for reading the pointer's value
on the write side or outside all locks.

3. Although it can be used on the write side, rcu_dereference_protected
should be used. So, it's for reading the pointer's value outside all
locks.

> 
> 
> 	lock(writer_lock);
> 	/* no need for rcu_dereference() because we have writer lock */
> 	ptr = rcu_access_pointer(obj->ptr);
> 	ptr->something = 1;
> 	unlock(writer_lock);

Here I totally agree with you, this isn't a valid usage, but I think
the documentation comment doesn't recommend any usage under the write
lock.

It's just a recommendation, though, so if you prefer rcu_access_pointer
to save a few lines of code, I can switch to it. We'll just lose extra
checks and add an unnecessary READ_ONCE under the hood.

> 
> It's still perfectly fine to use access_pointer as intended on 
> the write side, which is just checking the value of the pointer, 
> not deferencing it:

Sure, it's correct, but adds an extra READ_ONCE, which is not needed
under the lock, and skips the lockdep check, which is rather redundant
in this case:

down_write(&lock);
ptr = rcu_dereference_protected(rcu_ptr, lockdep_is_held(&lock));
...
up_write(&lock);

but is more meaningful in this piece of code:
	
/* Safe, because this is the destroy flow, refcount is 0, so
 * tls_device_down can't store this field in parallel.
 */
netdev = rcu_dereference_protected(ctx->netdev,
				   !refcount_read(&ctx->refcount));

async_cleanup = netdev && ctx->tx_conf == TLS_HW;

> 
> 	lock(writer_lock);
> 	if (rcu_access_pointer(obj->ptr) == target)
> 		so_something(obj);
> 	unlock(writer_lock);

Powered by blists - more mailing lists