Messages in this thread | ![/](/images/icornerl.gif) | | From | Linus Torvalds <> | Date | Tue, 18 Aug 2020 10:51:33 -0700 | Subject | Re: [RFC][PATCH] seccomp: Fail immediately if any thread is performing an exec |
| |
On Mon, Aug 17, 2020 at 12:11 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > +static void set_in_execve(bool in_execve) > +{ > + struct task_struct *me = current; > + spinlock_t *lock = &me->sighand->siglock; > + > + spin_lock_irq(lock); > + me->in_execve = in_execve; > + spin_unlock_irq(lock); > +}
No. This is complete voodoo programming. Code like this makes absolutely zero sense.
Using a lock to serialize a single write is completely bogus. Yes, it can be required if the field is a bitfield and you want to protect the other bits in the word, but then you shouldn't be using a bitfield.
It adds zero serialization that a WRITE_ONCE/READ_ONCE pair doesn't add. The other side will get either the old or the new value regardless of the locking, so the locking is completely worthless garbage.
At best it's just wasted CPU time. At worst, it confuses people about what the locking means and results in bugs down the line. Don't do things like this.
Locking for reading or writing a single value makes no sense. Locking is only useful when there is a coherency issue and you have a *sequence* of writes.
If you have a single value that acts as a flag, use READ_ONCE/WRITE_ONCE to show that there's no relevant locking. In fact, better yet, use "smp_store_release()" to set the flag and "smp_load_acquire()" to read it, and then you get the read/write once semantics _and_ an ordering between the "I have started doing X, everything I've done up until this point is now guaranteed to be visible to whoever reads this value".
Linus
PS. Yes, there are situations where you want to lock around a single write because of the serialization it guarantees for things around the locked code. So you can have valid "lock-write-unlock" sequences. But then you'd better have a BIG HONKING COMMENT about what the hell you're really serializing, because it's not the single write, it's something bigger.
| ![\](/images/icornerr.gif) |