>> 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
