Fwd: FW: [PATCH 04/31] nds32: Exception handling

[Vincent Chen]

>> From: Greentime Hu <greentime@xxxxxxxxxxxxx>
>>
>> Signed-off-by: Vincent Chen <vincentc@xxxxxxxxxxxxx>
>> Signed-off-by: Greentime Hu <greentime@xxxxxxxxxxxxx>
>>  arch/nds32/mm/alignment.c       |  564 +++++++++++++++++++++++++++++++++++++++
>
>> diff --git a/arch/nds32/mm/alignment.c b/arch/nds32/mm/alignment.c new
>> file mode 100644 index 0000000..05589e7
>> --- /dev/null
>> +++ b/arch/nds32/mm/alignment.c
>
>> +static int mode = 0x3;
>> +module_param(mode, int, S_IWUSR | S_IRUGO);
>
>It's an interesting question how to best handle alignment faults, both in
>kernel and user mode. While it can help for debugging to have the handler,
>I'd argue that you are better off in the long run not fixing up the faults
>automatically but to modify the code that triggers them instead.
>
>How about making the faults disabled by default?

Thanks
OK, I will disable alignment fault handling by default in next version patch

>> +static int _do_unaligned_access(unsigned long entry, unsigned long addr,
>> +                               unsigned long type, struct pt_regs *regs)
>> +{
>> +       unsigned long inst;
>> +       int ret = -EFAULT;
>> +
>> +       if (user_mode(regs)) {
>> +               /* user mode */
>> +               if (!va_present(current->mm, addr))
>> +                       return ret;
>> +       } else {
>> +               /* kernel mode */
>> +               if (!va_kernel_present(addr))
>> +                       return ret;
>> +       }
>
>This looks racy, the address might be present when you get here, but not
>later when you actually access it. I think what you need here is something
>like ARM does with get32_unaligned_check() etc and their fixup tables.

Thanks.
I will follow your suggestion to modify it


>> +       inst = get_inst(regs->ipc);
>> +
>> +       DEBUG(mode & 0x04, 1,
>> +             "Faulting Addr: 0x%08lx, PC: 0x%08lx [ 0x%08lx ]\n", addr,
>> +             regs->ipc, inst);
>> +
>> +       if ((user_mode(regs) && (mode & 0x01))
>> +           || (!user_mode(regs) && (mode & 0x02))) {
>> +
>> +               mm_segment_t seg = get_fs();
>> +
>> +               set_fs(KERNEL_DS);
>> +
>> +               if (inst & 0x80000000)
>> +                       ret = do_16((inst >> 16) & 0xffff, regs);
>> +               else
>> +                       ret = do_32(inst, regs);
>> +
>> +               set_fs(seg);
>> +       }
>> +
>> +       return ret;
>> +}
>
>Doesn't this allow user space to read all of kernel memory simply by
>passing unaligned addresses?

Thanks
I will fix it in next version patch.


>> +static const struct file_operations fops = {
>> +       .open = simple_open,
>> +       .read = proc_alignment_read,
>> +       .write = proc_alignment_write,
>> +};
>
>This should really be a sysctl rather than an open-coded procfs file,
>for consistency with other architectures.
>
>Please have a look at that interface on other architectures and pick
>whatever the majority do.

OK. I will use sysctl instaed of procfs to control this alignment
correction feature.
Thanks

>
>     Arnd

Best regards
Vincent