Messages in this thread Patch in this message | | | Date | Thu, 6 Jan 2011 16:45:39 +0100 | From | Frederic Weisbecker <> | Subject | Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry |
| |
On Thu, Jan 06, 2011 at 03:18:19PM +0000, Jan Beulich wrote: > >>> On 06.01.11 at 15:51, Frederic Weisbecker <fweisbec@gmail.com> wrote: > > From the x86_64 low level interrupt handlers, the frame pointer is > > saved in pt_regs in the wrong place, in the offset of rbx. > > > > rbx is not part of the irq partial saved registers, so it's not > > critical, but later code that uses get_irq_regs() to get the > > interrupted frame pointer instead get a stale rbx value, causing > > unwinding code to fail miserably. > > Code using get_irq_regs() can't rely on the not explicitly > saved registers' fields of pt_regs anyway; you now fix this > for %rbp, but someone else might look at a different > register and want that to be saved too. You just shouldn't, > and the fact that %rbp happens to be saved at all shouldn't > be taken to mean you can access it via the provided > pt_regs pointer. This saving of %rbp could go away or be > done in a different way at any point.
Right. That was something I was wondering about: is rbp saved here as the beginning of the proc stack, or was is supposed to be part of the pt_regs although mistakenly recorded in rbp?
So given what you are explaining me, it rather seem it wasn't supposed to be of part pt_regs and it's there because it begins the frame of the irq handler.
I agree that in the case of common irqs, rbp is not supposed to be part of saved regs. I think may be we can change that semantic though as it requires very few changes. In fact the only added overhead is that addq below.
I'm waiting for opinions though.
> > @@ -808,6 +813,8 @@ ret_from_intr: > > TRACE_IRQS_OFF > > decl PER_CPU_VAR(irq_count) > > leaveq > > + /* we did not save rbx, restore only from ARGOFFSET */ > > + addq $8, %rsp > > CFI_RESTORE rbp > > CFI_DEF_CFA_REGISTER rsp > > CFI_ADJUST_CFA_OFFSET -8 > > *If* the patch was to be taken anyway, I would strongly suggest > getting this mis-insertion fixed first: CFI annotations belong to the > immediately preceding instruction, and hence you must not insert > new instructions between an existing one and its annotation.
Good to know, will fix.
> Furthermore, an adjustment to %rsp (when it serves as the frame > pointer as is the case here, though would be better visible if the > added instruction was at the right place) needs to be annotated > itself.
Hmm, I'm not familiar with those CFI adjustments. Before we had:
leaveq CFI_RESTORE rbp CFI_DEF_CFA_REGISTER rsp CFI_ADJUST_CFA_OFFSET -8
So CFI_RESTORE means rbp has now the value of the base frame of the calling frame (the base frame pointer of the interrupted proc) ?
And what follows means that rsp-8 points to the return address? But it doesn't seem to be the case, the return address here beeing rip stored in pt_regs...
I'm confused.
| |