Messages in this thread | | | Date | Wed, 29 Mar 2023 15:58:06 +0200 | From | Petr Mladek <> | Subject | buffer write race: Re: [PATCH printk v1 09/18] printk: nobkl: Add print state functions |
| |
On Thu 2023-03-02 21:02:09, John Ogness wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > Provide three functions which are related to the safe handover > mechanism and allow console drivers to denote takeover unsafe > sections: > > - console_can_proceed() > > Invoked by a console driver to check whether a handover request > is pending or whether the console was taken over in a hostile > fashion. > > --- a/kernel/printk/printk_nobkl.c > +++ b/kernel/printk/printk_nobkl.c > @@ -947,6 +947,145 @@ static void cons_free_percpu_data(struct console *con) > con->pcpu_data = NULL; > } > > +/** > + * console_can_proceed - Check whether printing can proceed > + * @wctxt: The write context that was handed to the write function > + * > + * Returns: True if the state is correct. False if a handover > + * has been requested or if the console was taken > + * over. > + * > + * Must be invoked after the record was dumped into the assigned record > + * buffer
The word "after" made me think about possible races when the record buffer is being filled. The owner might loose the lock a hostile way during this action. And we should prevent using the same buffer when the other owner is still modifying the content.
It should be safe when the same buffer might be used only by nested contexts. It does not matter if the outer context finishes writing later. The nested context should not need the buffer anymore.
But a problem might happen when the same buffer is shared between more non-nested contexts. One context might loose the lock a hostile way. The other context might get the access after the hostile context released the lock.
NORMAL and PANIC contexts are safe. These priorities have only one context and both have their own buffers.
A problem might be with EMERGENCY contexts. Each CPU might have its own EMERGENCY context. We might prevent this problem if we do not allow to acquire the lock in EMERGENCY (and NORMAL) context when panic() is running or after the first hostile takeover.
If we want to detect these problems and always be on the safe side, we might need to add a flag 1:1 connected with the buffers.
We either could put a flag into struct printk_buffers. Or we could bundle this struct into another one for the atomic consoles. I mean something like:
struct printk_atomic_buffers { struct printk_buffers pbufs; atomic_t write_lock; }
And use it in cons_get_record():
#define PRINTK_BUFFER_WRITE_LOCKED_VAL 1
static int cons_get_record(struct cons_write_context *wctxt) { int ret = 0;
/* * The buffer is not usable when another write context * is still writing the record and lost the lock a hostille * way. */ if (WARN_ON_ONCE(cmpxchg_acquire(&wctxt->pabufs->write_lock, 0, PRINTK_BUFFER_WRITE_LOCKED_VAL) != 0)) { return -EBUSY; }
// Fill the buffers
if (no new message) { ret = -ENOENT; goto unlock; }
unlock: /* Release the write lock */ atomic_set_release(&wctxt->pabufs->write_lock, 0); return ret; }
Note: This is related to the discussion about the 7th patch but it is not the same.
This mail is only about using the same buffer for the same console.
The other discussion was also about using the same buffer for more consoles. It is even more problematic because each console uses its own lock.
It means that we would still need separate buffer for each interrupt context. Nested context might be able to get the lock for another console a regular way, see https://lore.kernel.org/all/ZBndaSUFd4ipvKwj@alley/
> + * and at appropriate safe places in the driver. For unsafe driver > + * sections see console_enter_unsafe(). > + * > + * When this function returns false then the calling context is not allowed > + * to go forward and has to back out immediately and carefully. The buffer > + * content is no longer trusted either and the console lock is no longer > + * held. > + */ > +bool console_can_proceed(struct cons_write_context *wctxt) > +{
Best Regards, Petr
| |