Wonky Typecasting

Tinkering with strange forces, Steven Rostedt felt that a particular struct in the kernel should be tucked away into a private file, and the parts of the kernel using that struct should only be given a pointer to it. However, some (but not all) compilers started to complain about those in-kernel users trying to dereference the pointer (i.e., to see what it pointed to, which Steven had rendered hidden and inaccessible).

However, it was a little more complicated than that. The code that dereferenced the pointer wasn't targeting this specific pointer; it would simply dereference any pointer that was handed to it. But in this case, the struct's pointer was a secret hidden struct and was thus different from the types of pointers that code was usually handed, so … boom.

To fix this, Steven went through a big pile of kernel code and changed a lot of pointer declarations so that the dereferencing code wouldn't complain anymore. He didn't change it to be more accurate; he just changed it so the dereferencing process itself wouldn't care what was at the end of its request.

Specifically, he changed all occurrences of

typeof(*p) *_p

to

typeof(p) _p

On the mailing list, Steven said he "Also had to update a lot of the function pointer initialization in the networking code, as a function address must be passed as an argument in RCU_INIT_POINTER() and not just the function name."

Simple, straightforward, and unobjectionable in any way, right?

Mathieu Desnoyers pointed out that Steven's change "removes validation that @p is indeed a pointer, so a user might mistakenly try to use rcu_dereference() on an integer, and get away with it. I'm not sure we want to loosen this check. I wonder if there might be another way to achieve the same check without requiring the structure to be declared."

So Mathieu wasn't totally opposed, but he did notice the swirling madness gathering around Steven's cauldron.

Steven replied:

"Is that really an issue? Because you would be assigning it to an integer.

x = rcu_dereference_raw(y);

"And that just makes 'x' a copy of 'y' and not really a reference to it, thus if you don't have a pointer, it's just a fancy READ_ONCE(y)."

But Mathieu quoted from the Documentation/RCU/arrayRCU.rst file, which said explicitly:

"It might be tempting to consider use of RCU to instead protect the index into an array, however, this use case is not supported. The problem with RCU-protected indexes into arrays is that compilers can play way too many optimization games with integers, which means that the rules governing handling of these indexes are far more trouble than they are worth. If RCU-protected indexes into arrays prove to be particularly valuable (which they have not thus far), explicit cooperation from the compiler will be required to permit them to be safely used."

Paul E. McKenney applauded Mathieu's documentation research abilities and quipped, "On the other hand, I am starting to believe that explicit cooperation from compilers might actually be forthcoming in my lifetime, so there might well be that …."

At this point, Linus Torvalds came into the conversation with a disgruntled critique. He said:

"This is a sign of why we did it the way we did with that odd 'typeof(*p)*' thing in the first place.

"The thing is, in any normal C, the function name should just stand in for the pointer to the function, so having to add a '&' to get the function pointer is somehow odd.

"So I think you should just expose your type to anybody who uses a pointer to it."

Steven shrugged, put the lid back on his cauldron, and said:

"I'll go punt and just expose the structure. It's not a big deal, but I like abstraction of structures when they can be, just to keep from the temptation of tweaking them directly, and causing updates later to be more difficult.

"Too bad that the failure here is not RCU or the macros, but what I would call a bug in a specific compiler."

Linus said, "I'm admittedly surprised that something like this would be a 'different compiler versions' issue. But 'typeof()' isn't exactly standard C, so the fact that some version of gcc did something slightly different is annoying but I guess not _that_ surprising."

However! If anyone expected the story to have a happy ending, they were soon disabused, at least for now.

Jan Engelhardt unlidded his own vaporous cauldron and ladled out some horrific insanity that he felt might solve at least part of the per-compiler issues dereferencing Steven's nasty pointer. He said:

#define static_cast(type, expr) ((struct { type x; }){(expr)}.x)
typeof(p) p1 = (typeof(p) __force)  static_cast(void *, READ_ONCE(p));

"Let the name not fool you; it's absolutely _not_ the same as C++'s static_cast, but still: it does emit a warning when you do pass an integer, which is better than no warning at all in that case."

Steven, donning his blue hat with yellow stars once again, replied, "Are you suggesting I should continue this exercise?"

To which Jan with mischievous encouragement replied, "Why not?"

Steven, rolling up the sleeves of his robes, said, "I may try it, because exposing the structure I want to hide, is pulling out a lot of other crap with it."

This was why he'd been trying to hide that struct in the first place. The struct, itself, contained pointers to other structs and unions that might also have to be exposed, not to mention various locks and types. It was like Chihiro in Spirited Away pulling that last little cork out of the Stink Spirit, and Steven feared all the pollution ever dumped into an entire river might come pouring out.

Linus said, "One option is just 'don't do rcu_access of a pointer that you're not supposed to touch in a file that isn't supposed to touch it'."

He went on:

"IOW, why are you doing that

pid_list = rcu_dereference_sched(tr->function_pids);

"in a place that isn't supposed to look at the pid_list in the first place?

"Yeah, yeah, I see how you just pass it to trace_ignore_this_task() as an argument, but maybe the real fix is to just pass that trace_array pointer instead?

"IOW, if you want to keep that structure private, maybe you really just shouldn't have non-private users of it randomly doing RCU lookups of it?"

To which Steven replied, "The problem is, the RCU isn't for touching it, it is for knowing it exists."

Steven went on to explain some of the deeper, darker recesses of his potion's efficacy. The whole issue stemmed from the fact that he was trying to do a bigger change, and "this was the best 'incremental' approach I had, as the code is currently all just open coded."

It's a longstanding practice for the top developers to try to break their giant dragons into readable minnows so their patches each do something small and clear – Linus once pointed to Alexander Viro as an exemplar of this style of patch submission. Ever since then, it's been every developer's common practice to at least make the attempt.

But in this case, Steven said, "the logic to synchronize updates is left to the user not the pid list itself." And therefore, a whole bunch of data allocation, swapping values, and other weird sorcery taking place that deep in the code, Steven said, was not actually related to what he was trying to do – it just happened to be there standing in his way. He wanted to avoid it as gracefully as possible, which meant rearranging the guts of that beast.

Steven summed up his situation by saying, "I don't believe there's anything wrong with returning a pointer of one type, and then typecasting it to a pointer of another type. Is there? As long as whoever uses the returned type does nothing with it."

And that was the point – he didn't actually want to do anything at that place in the code. He just wanted something that wasn't supposed to happen anyway – those annoying compiler errors – to stop happening.

But Linus, at this point, put his foot down. He said:

"Just stop doing this.

"Dammit, just include the header file that defines the type in the places that you use the thing.

"Because, yes, there is a LOT wrong with just randomly casting pointers that you think have the 'wrong type'. You're basically taking it on yourself to lie to the compiler, and intentionally breaking the type system, because you have some completely bogus reason to hide a type.

"We don't hide types in the kernel for no good reason.

"You are literally talking about making things worse, for a reason that hasn't even been explained, and isn't valid in the first place. Nothing else in the kernel has had a problem just declaring the damn type.

"If there was some clean and simple solution to the compiler warning problem, that would be one thing. But when you think you need to change core RCU macros, or lie to the compiler about the type system, at that point it's not some clean and simple fix any more. At that point you're literally making things worse than just exposing the type."

At which point Steven returned to his original pre-Jan-incitement concession, saying, "Fine, I'll just create a separate header file with all that is needed and add it to the include. At least that way, it doesn't muck up the rest of the header file."

And Jan, at this point siding perhaps with Linus or perhaps with the mere concept of seeing how much smoke could pour out of as many cauldrons at once, pointed to the actual C++ reference, regarding Steven's remark that there couldn't be anything wrong with returning a pointer of one type and then casting it to the pointer of another, especially if the user of that pointer just threw it away.

Jan said that what Steven wanted was actually illegal according to the standard. The reference material said, "If a pointer to object is converted to a pointer to void and back, its value compares equal to the original pointer." And, "No other guarantees are offered."

So, at this point, Linus looked at the ground, kicked with the toe of his shoe into the dirt a little, hands behind his back, and said:

"Well, we happily end up casting pointers to 'unsigned long' and back, and doing bit games on the low bits of a pointer value.

"So it's not like the kernel deeply cares about theoretical portability.

"But I do discourage casting when not required, just because as much static type checking we can possibly have is good when we can do it."

At this point, the conversation flickered and died, with Steven saying, finally, "I solved this by creating a separate header for the nasty structure, but it's still public for all references."

The thing I love about this whole conversation is just the sheer weirdness of what Steven was trying to do, and the strange way it bit him in the ass. He wanted to hide some highly messy code from the rest of the kernel, as a way to simplify the lives of other maintainers, so they wouldn't feel any urge to tinker in that code while trying to fix anything nearby.

However, by doing this in a way that seemed to make sense to him, Steven triggered something strange and unexpected in the way a whole bunch of compilers were implemented differently from each other. And as Linus said, it's not like the kernel is a standards freak. Linus breaks standards whenever, in his judgment, the standard is wrong. But that's hardly a clear bit of guidance. And not Steven or anyone else can be faulted for going around the rules, or trying to, when something seems to need it.

So Steven was willing to abandon the attempt to hide the crazy pieces until Jan dared him to continue, but in fact it was just too nightmarish. It was probably somewhat fun (I would guess) for Steven to make the attempt and learn things he hadn't known before. But it was also a lot of work for nothing, since Linus finally wanted something simpler, even if that meant exposing the ugly thing that other developers might get confused by later.