Re: [PATCH 1/2] mm/vmalloc: export is_vmalloc_or_module_addr

[Richard Yao]

On 02/08/2014 03:06 PM, Linus Torvalds wrote:
> On Sat, Feb 8, 2014 at 11:58 AM, Richard Yao <ryao@xxxxxxxxxx> wrote:
>>
>> My apologies for that. Here is the backtrace:
>>
>> [<ffffffff814878ce>] p9_virtio_zc_request+0x45e/0x510
>> [<ffffffff814814ed>] p9_client_zc_rpc.constprop.16+0xfd/0x4f0
>> [<ffffffff814839dd>] p9_client_read+0x15d/0x240
>> [<ffffffff811c8440>] v9fs_fid_readn+0x50/0xa0
>> [<ffffffff811c84a0>] v9fs_file_readn+0x10/0x20
>> [<ffffffff811c84e7>] v9fs_file_read+0x37/0x70
>> [<ffffffff8114e3fb>] vfs_read+0x9b/0x160
>> [<ffffffff81153571>] kernel_read+0x41/0x60
>> [<ffffffff810c83ab>] copy_module_from_fd.isra.34+0xfb/0x180
> 
> So copy_module_from_fd() does read into a vmalloc'ed buffer (which
> isn't pretty, but at least it's not like using some statically
> allocated module data), but that's a regular vmalloc() afaik.
> 
> So I don't see the need for "is_vmalloc_or_module_addr()". The regular
> "is_vmalloc_addr()" should be fine, and *is* usable from modules (it's
> inline, not exported, but it comes to the same thing wrt module use),
> exactly because we have traditionally allowed vmalloc'ed memory to be
> used.
> 
> So is there some reason why it's not just using that simpler function?

My first instinct was to use "is_vmalloc_addr()" as you suggest.
However, is_vmalloc_addr() only applies to the vmalloc region. While all
architectures load kernel modules into virtual memory (to my knowledge),
some architectures do not load them into the vmalloc region.
is_vmalloc_or_module_addr() contains a comment that clearly states this:

int is_vmalloc_or_module_addr(const void *x)
{
        /*
         * ARM, x86-64 and sparc64 put modules in a special place,
         * and fall back on vmalloc() if that fails. Others
         * just put it in the vmalloc space.
         */
#if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
        unsigned long addr = (unsigned long)x;
        if (addr >= MODULES_VADDR && addr < MODULES_END)
                return 1;
#endif
        return is_vmalloc_addr(x);
}

My inspection of the actual code agreed with that comment (at least for
S390), so I decided against using "is_vmalloc_addr()" here.

Attachment: signature.asc
Description: OpenPGP digital signature