| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Nov 2022 10:43:48 -0800
From: Deepak Gupta <debug@...osinc.com>
To: Conor.Dooley@...rochip.com
Cc: aou@...s.berkeley.edu, jan.kiszka@...mens.com, kbingham@...nel.org,
linux-kernel@...r.kernel.org, palmer@...belt.com,
paul.walmsley@...ive.com, ajones@...tanamicro.com,
linux-riscv@...ts.infradead.org, bjorn@...nel.org,
atishp@...osinc.com
Subject: Re: [PATCH v3] scripts/gdb: add lx_current support for riscv
On Tue, Nov 15, 2022 at 06:06:34PM +0000, Conor.Dooley@...rochip.com wrote:
>Hey Deepak,
>
>On 15/11/2022 17:49, Deepak Gupta wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> Since I am new to all this. I've had some oversight and am still learning the flow.
>> Rest inline.
>
>No worries chief. Worth noting is that this mail came in html
>form, which the mailing lists reject. Noone outside of the
>direct CC list will see this mail. May be worth asking some
>of the other Rivos lads how they do their plain text emailing.
>
>ik Palmer's got his hand rolled stuff, so maybe he's not the
>best to ask - but try Bjorn or Atish?
Sending this time from mutt. Hopefully no bounces.
>
>>
>> On Tue, Nov 15, 2022 at 6:38 AM Conor Dooley <conor.dooley@...rochip.com <mailto:conor.dooley@...rochip.com>> wrote:
>>
>> Hey Deepak,
>>
>> On Tue, Nov 15, 2022 at 12:49:23AM -0800, Deepak Gupta wrote:
>> > csr_sscratch CSR holds current task_struct address when hart is in
>> > user space. Trap handler on entry spills csr_sscratch into "tp" (x2)
>> > register and zeroes out csr_sscratch CSR. Trap handler on exit reloads
>> > "tp" with expected user mode value and place current task_struct address
>> > again in csr_scratch CSR.
>> >
>> > This patch assumes "tp" is pointing to task_struct. If value in
>> > csr_scratch is numerically greater than "tp" then it assumes csr_scratch
>>
>> nit: s/scratch/sscratch/ ?
>>
>>
>> Will fix it.
>>
>>
>>
>> > is correct address of current task_struct. This logic holds when
>> > - hart is in user space, "tp" will be less than csr_scratch.
>> > - hart is in kernel space but not in trap handler, "tp" will be more
>> > than csr_scratch (csr_scratch being equal to 0).
>> > - hart is executing trap handler
>> > - "tp" is still pointing to user mode but csr_scratch contains
>> > ptr to task_struct. Thus numerically higher.
>> > - "tp" is pointing to task_struct but csr_scratch now contains
>> > either 0 or numerically smaller value (transiently holds
>> > user mode tp)
>> >
>> > Patch also adds new cached type "ulong" in scripts/gdb/linux/utils.py
>> >
>> > Signed-off-by: Deepak Gupta <debug@...osinc.com <mailto:debug@...osinc.com>>
>>
>> I noticed when looking into patchwork complaining about checkpatch
>> errors in v2, that b4 had actually downloaded v3 but I could not see
>> this patch on the RISC-V list.
>>
>>
>> I'll make sure to add the risc-v list on the next spin up.
>>
>>
>> I don't see a changelog anywhere here
>> from v2 either
>>
>>
>> I had been taking inputs and squashing commits on my end.
>> You want me to send a changelog of changes between versions of patches.
>
>Yeah, it's nice to say something like:
>v2 -> v3:
>- reworded commit message
>- fixed compile error in bar.c if !CONFIG_FOO
>
>Makes it easier for reviewers to see what changed between
>versions.
>
>>
>>
>> , nor did you pick up Drew's Reviewed-by.
>>
>>
>> I should've done that. My mistake and apologize.
>> I'll fix it in my next submission.
>>
>>
>>
>> What's the story there?
>>
>> One really minor thing below. Should be able to fix it up trivially up
>> & submit a v4, CCing the linux-riscv list.
>>
>> > ---
>> > scripts/gdb/linux/cpus.py | 15 +++++++++++++++
>> > scripts/gdb/linux/utils.py | 5 +++++
>> > 2 files changed, 20 insertions(+)
>> >
>> > diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py
>> > index 15fc4626d236..ca5215a660c7 100644
>> > --- a/scripts/gdb/linux/cpus.py
>> > +++ b/scripts/gdb/linux/cpus.py
>> > @@ -173,6 +173,21 @@ def get_current_task(cpu):
>> > else:
>> > raise gdb.GdbError("Sorry, obtaining the current task is not allowed "
>> > "while running in userspace(EL0)")
>> > + elif utils.is_target_arch("riscv"):
>> > + current_tp = gdb.parse_and_eval("$tp")
>> > + scratch_reg = gdb.parse_and_eval("$sscratch")
>> > +
>> > + # by default tp points to current task
>> > + current_task = current_tp.cast(task_ptr_type)
>> > +
>> > + # scratch register is set 0 in trap handler after entering kernel.
>> > + # When hart is in user mode, scratch register is pointing to task_struct.
>> > + # and tp is used by user mode. So when scratch register holds larger value
>> > + # (negative address as ulong is larger value) than tp, then use scratch register.
>> > + if (scratch_reg.cast(utils.get_ulong_type()) > current_tp.cast(utils.get_ulong_type())):
>> ^^
>> extra space here?
>>
>>
>> I don't see the space in the patch. Can you clarify which space you're talking about here?
>
>There's a double space between the > and current_tp.
>I put a ^^ under it, but if you've not got a monospace font, which since
>you're replying in html you probably don't, it may not align for you.
>
>Hope that helps,
>Conor.
Yes can see it now.
>
>>
>>
>>
>>
>> > + current_task = scratch_reg.cast(task_ptr_type)
>> > +
>> > + return current_task.dereference()
>> > else:
>> > raise gdb.GdbError("Sorry, obtaining the current task is not yet "
>> > "supported with this arch")
>> > diff --git a/scripts/gdb/linux/utils.py b/scripts/gdb/linux/utils.py
>> > index 1553f68716cc..ddaf3089170d 100644
>> > --- a/scripts/gdb/linux/utils.py
>> > +++ b/scripts/gdb/linux/utils.py
>> > @@ -35,12 +35,17 @@ class CachedType:
>> >
>> >
>> > long_type = CachedType("long")
>> > +ulong_type = CachedType("ulong")
>> > atomic_long_type = CachedType("atomic_long_t")
>> >
>> > def get_long_type():
>> > global long_type
>> > return long_type.get_type()
>> >
>> > +def get_ulong_type():
>> > + global ulong_type
>> > + return ulong_type.get_type()
>> > +
>> > def offset_of(typeobj, field):
>> > element = gdb.Value(0).cast(typeobj)
>> > return int(str(element[field].address).split()[0], 16)
>> > --
>> > 2.25.1
>>
>
Powered by blists - more mailing lists