Messages in this thread | | | Date | Thu, 15 Feb 2024 13:56:21 -0500 | Subject | Re: [PATCH v8 2/2] lib: checksum: Use aligned accesses for ip_fast_csum and csum_ipv6_magic tests | From | John David Anglin <> |
| |
On 2024-02-15 1:17 p.m., Guenter Roeck wrote: > On 2/15/24 09:25, John David Anglin wrote: > [ ... ] >>>>> Source: >>>>> >>>>> static bool pc_is_kernel_fn(unsigned long pc, void *fn) >>>>> { >>>>> return (unsigned long)dereference_kernel_function_descriptor(fn) == pc; >>>> This looks wrong to me. Function descriptors should always be 8-byte aligned. I think this >>>> routine should return false if fn isn't 8-byte aligned. >>> >>> Below you state "Code entry points only need 4-byte alignment." >>> >>> I think that contradicts each other. Also, the calling code is, >>> for example, >>> pc_is_kernel_fn(pc, syscall_exit) >>> >>> I fail to see how this can be consolidated if it is ok >>> that syscall_exit is 4-byte aligned but, at the same time, >>> must be 8-byte aligned to be considered to be a kernel function. >> In the above call, syscall_exit is treated as a function pointer. It points to an 8-byte aligned >> function descriptor. The descriptor holds the actual address of the function. It only needs >> 4-byte alignment. >> >> Descriptors need 8-byte alignment for efficiency on 64-bit parisc. The pc and gp are accessed >> using ldd instructions. >> > > Maybe code such as > pc_is_kernel_fn(pc, syscall_exit) > is wrong because syscall_exit doesn't point to a function descriptor > but to the actual address. The code and comments in arch/parisc/kernel/unwind.c It depends on how syscall_exit is declared. unwind.c lies the type of handle_interruption, etc:
extern void * const handle_interruption; extern void * const ret_from_kernel_thread; extern void * const syscall_exit; extern void * const intr_return; extern void * const _switch_to_ret; #ifdef CONFIG_IRQSTACKS extern void * const _call_on_stack; #endif /* CONFIG_IRQSTACKS */
This should yield actual addresses. > is for sure confusing because it talks about not using > dereference_kernel_function_descriptor() to keep things simple but then calls > dereference_kernel_function_descriptor() anyway. Maybe it should just be > if (pc == syscall_exit) > instead. Looks like. > > The entire code is really odd anyway. > > ptr = dereference_kernel_function_descriptor(&handle_interruption); > if (pc_is_kernel_fn(pc, ptr)) { > > and then pc_is_kernel_fn() dereferences it again. Weird. > > It looks like commit 8e0ba125c2bf ("parisc/unwind: fix unwinder when > CONFIG_64BIT is enabled") might have messed this up. No idea how to fix > it properly, though. This is Helge's code... I'll let him fix it.
Dave
-- John David Anglin dave.anglin@bell.net
| |