netdev - Re: [PATCH net-next v4 3/5] ipv6: ioam: IOAM Generic Netlink API

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]

Message-ID: <20210530131328.55941d22@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date:   Sun, 30 May 2021 13:13:28 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Justin Iurman <justin.iurman@...ege.be>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, tom@...bertland.com
Subject: Re: [PATCH net-next v4 3/5] ipv6: ioam: IOAM Generic Netlink API

On Sun, 30 May 2021 13:18:24 +0200 (CEST) Justin Iurman wrote:
> >> +	sc = ns->schema;
> >> +	err = rhashtable_remove_fast(&nsdata->namespaces, &ns->head,
> >> +				     rht_ns_params);
> >> +	if (err)
> >> +		goto out_unlock;
> >> +
> >> +	if (sc)
> >> +		sc->ns = NULL;  
> > 
> > the sc <> ns pointers should be annotated with __rcu, and appropriate
> > accessors used. At the very least the need READ/WRITE_ONCE().  
> 
> I thought that, in this specific case, the mutex would be enough.

For writing, but not for preventing data races between writer and
reader.

> Note that rcu is used everywhere else for both of them (see patch
> #2). Could you explain your reasoning? 

We need to make sure the compiler doesn't play tricks. LWN explains
this in detail (worth reading in its own right):

https://lwn.net/Articles/793253/
https://lwn.net/Articles/799218/

Looking at the code again, I think it's even worse, I see:

ioam6_fill_trace_data()
	if (ns->schema)
		sclen += ns->schema->len / 4;

__ioam6_fill_trace_data()
	if (!ns->schema) {
		*(__be32 *)data = cpu_to_be32(IOAM6_EMPTY_u24);
	} else {
		*(__be32 *)data = ns->schema->hdr;
		data += sizeof(__be32);

		memcpy(data, ns->schema->data, ns->schema->len);

Value of schema can change in between. So it may had been NULL when we
checked for the length calculation, and non-NULL when we were writing
data. ns->schema should be dereferenced once, and result passed around.

> So I guess your comment also applies to other functions here
> (add/del, etc), where either ns or sc is accessed, right?

Correct.