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:

quoted

quoted

+	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.