Re: [bug report] x86/split_lock: Make life miserable for split lockers

[Tom Lendacky]

+Joerg

On 11/28/23 11:40, Tony Luck wrote:
> On Tue, Nov 28, 2023 at 04:12:24PM +0300, Dan Carpenter wrote:
> > Hello Tony Luck,
> >
> > The patch b041b525dab9: "x86/split_lock: Make life miserable for
> > split lockers" from Mar 10, 2022 (linux-next), leads to the following
> > Smatch static checker warning:
> >
> > 	arch/x86/kernel/cpu/intel.c:1179 split_lock_warn()
> > 	warn: sleeping in atomic context
> >
> > arch/x86/kernel/cpu/intel.c
> >      1158 static void split_lock_warn(unsigned long ip)
> >      1159 {
> >      1160         struct delayed_work *work;
> >      1161         int cpu;
> >      1162
> >      1163         if (!current->reported_split_lock)
> >      1164                 pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address: 0x%lx\n",
> >      1165                                     current->comm, current->pid, ip);
> >      1166         current->reported_split_lock = 1;
> >      1167
> >      1168         if (sysctl_sld_mitigate) {
> >      1169                 /*
> >      1170                  * misery factor #1:
> >      1171                  * sleep 10ms before trying to execute split lock.
> >      1172                  */
> >      1173                 if (msleep_interruptible(10) > 0)
> >      1174                         return;
> >      1175                 /*
> >      1176                  * Misery factor #2:
> >      1177                  * only allow one buslocked disabled core at a time.
> >      1178                  */
> > --> 1179                 if (down_interruptible(&buslock_sem) == -EINTR)
> >      1180                         return;
> >      1181                 work = &sl_reenable_unlock;
> >      1182         } else {
> >      1183                 work = &sl_reenable;
> >      1184         }
> >      1185
> >      1186         cpu = get_cpu();
> >      1187         schedule_delayed_work_on(cpu, work, 2);
> >      1188
> >      1189         /* Disable split lock detection on this CPU to make progress */
> >      1190         sld_update_msr(false);
> >      1191         put_cpu();
> >      1192 }
> >
> > The call tree is:
> >
> > kernel_exc_vmm_communication() <- disables preempt
> > -> vc_raw_handle_exception()
> >     -> vc_forward_exception()
> >        -> exc_alignment_check()
> >           -> __exc_alignment_check()
> >              -> handle_user_split_lock()
> >                 -> split_lock_warn()
> >
> > I think maybe the mismatch is that kernel_exc_vmm_communication() calls
> > irqentry_nmi_enter(regs); which disable preemption but exc_alignment_check()
> > does local_irq_enable() which doesn't enable it.
>
> I think we need some arch/x86/kernel/sev.c expertise to explain the
> preemption requirements in that stack trace. Adding Tom Lendacky.

Adding Joerg as the original developer of this code.

I believe that irqentry_nmi_enter() is used so that we are ensured that 
the kernel can't be interrupted while using the per-CPU GHCB when entered 
from kernel-mode in order to avoid nested #VCs (except for an NMI). Joerg 
might have further insights since there was a lot of discussion around 
these changes.

I'm not sure if is possible, but I wonder if irqentry_nmi_exit() can be 
issued before forwarding the exception - or even delay forwarding the 
exception until after irqentry_nmi_exit().

Thanks,
Tom

> > Also why does arch/x86 not have a dedicated mailing list?
>
> Good question. X86 was once the default architecture. So everything went to
> linux-kernel@xxxxxxxxxxxxxxx. I'll add that to Cc: for this. But maybe
> it's time for an x86 specific list?
>
> > regards,
> > dan carpenter
>
> -Tony