Re: Thread Safety Analysis and the Linux kernel

On Wed, Mar 05, 2025 at 11:54:14PM +0000, Puchert, Aaron wrote:

> > 2. Basic alias analysis, e.g. when storing a pointer to a lock in a
> > function-local variable. [Complaint in:
> > https://lore.kernel.org/all/[email protected]/
> > -- quote: "Fix the analyzer instead."]
> 
> We already do this analysis for C++ references, and we might extend it
> to pointers, but with one restriction: an important aspect of
> references that we're relying on is that they're essentially immutable
> pointers, i.e. "T&" semantically behaves like "T* const". This makes
> references behave like SSA values, meaning we can just symbolically
> substitute the initializer. If we can ask the user to mimic references
> by making the local variable "const", we can apply the same logic. (I
> know that using "const" is a bit tricky in C, e.g. "T**" doesn't
> implicitly convert to "const T* const*" as it does in C++. However, I
> would hope that you can always add top-level "const".)
> 
> With "non-const" pointers we're unfortunately opening a can of worms:
> 
> struct S { int a, b; };
> 
> void f(struct S *s)
> {
> 	int *val = &s->a;
> 
> 	while (...) {
> 		do_something(val);
> 		val = &s->b;
> 	};
> }
> 
> The analysis walks the control-flow graph (CFG), and it wants to walk
> it only once. (We handle back edges by checking that the set of
> capabilities matches the set that we previously computed.) Here we
> have val == &s->a when we encounter the call to do_something. But then
> we see a new assignment and a back edge, so our assumption was wrong!
> 
> I don't think we want to make multiple rounds over the CFG. We're not
> trying to do symbolic execution. There are performance implications,
> reliability implications, and depending on how far we want to go, we
> might even run into the halting problem.
> 
> So if we can restrict this to "T* const", I don't see an issue with
> it, and it should arise naturally from the existing alias analysis for
> references.
> 
> Perhaps we can emit notes on -Wthread-safety-precise that suggest
> using "const" pointers or references in case we suspect alias analysis
> has fallen short.

So one of the most important use cases from my perspective is the
interaction with __attribute__((cleanup())), which we use to build
scope based guards in C.

And most of that might be fine with using 'T * const', but the support
for conditional locks explicitly needs to set the pointer NULL when the
acquire fails.

We have a macro used like:

DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))

Which expands to:

static inline void class_mutex_destructor(struct mutex **p)
{
	struct mutex *_T = *p;
	if (_T) { mutex_unlock(_T); }
}

static inline struct mutex * class_mutex_constructor(struct mutex *_T)
{
	struct mutex * t = ({ mutex_lock(_T); _T; });
	return t;
}

This is then typically used like:

	scoped_guard (mutex, &my_lock) {
		...
	}

Which in turn expands to:

	for (struct mutex *scope __attribute__((cleanup(class_mutex_destructor))) =
	     class_mutex_constructor(&my_lock); scope; ({ goto _label; }))
		if (0) {
_label:
			break;
		} else {

		...
	}

And here I can probably make the guard() thing add const like:

		struct mutex * const __UNIQUE_ID_guard_123 
			__attribute__((cleanup(class_mutex_destructor) =
				class_mutex_constructor(&my_lock);

But note that _constructor() is still laundering this through a
non-const version, and while the above could have:

	struct mutex * t const = ...

That no longer works for the case of:

DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))

which expands to have a constructor like:

static inline struct mutex * class_mutex_try_constructor(struct mutex *_T)
{
	struct mutex * t = ({ void *_t = _T; if (_T && !(mutex_trylock(_T))) _t = NULL; _t; });
	return t;
}

Notably that NULL assignment on lock failure is not happy with having
that const on.

Note that this combination of trylock and scoped_guard() results in not
taking the for-loop on failure. Eg:

	scoped_guard (mutex_trylock, &my_lock) {
		print("ponies\n");
	}

Will in fact only print 'ponies' when it got the lock.


And yes, all of this is fairly creative use of C, but hey, we get to
have scope based guards, which get rid of a lot of errors in error
paths.


So assuming we can annotate things like so:

static inline void class_mutex_destructor(struct mutex * const *p)
	__releases(*p) __no_capability_analysis
{
	struct mutex *_T = *p;
	if (_T) { mutex_unlock(_T); }
}

static inline struct mutex * class_mutex_constructor(struct mutex *_T)
	__acquires(_T) __no_capability_analysis
{
	struct mutex * t = ({ mutex_lock(_T); _T; });
	return t;
}

static inline struct mutex * class_mutex_try_constructor(struct mutex *_T)
	__const_acquires(nonnull, _T) __no_capability_analysis
{
	struct mutex * t = ({ void *_t = _T; if (_T && !(mutex_trylock(_T))) _t = NULL; _t; });
	return t;
}

the: struct mutex * const in the for() construct might just be enough.


Would this be sufficient consty for it to untangle this web?