Message-ID: <56A419C8.6090906@linaro.org>
Date:	Sun, 24 Jan 2016 00:24:40 +0000
From:	Kieran Bingham <kieran.bingham@...aro.org>
To:	Jan Kiszka <jan.kiszka@...mens.com>
Cc:	linux-kernel@...r.kernel.org, maxime.coquelin@...com,
	peter.griffin@...aro.org, lee.jones@...aro.org
Subject: Re: [PATCH 4/5] scripts/gdb: Add mount point list command

On 23/01/16 15:27, Jan Kiszka wrote:
> On 2016-01-20 12:15, Kieran Bingham wrote:
>> lx-mounts will identify current mount points based on the 'init_task'
>> namespace by default, as we do not yet have a kernel thread list
>> implementation to select the current running thread.
>>
>> Optionally, a user can specify a PID to list from that process'
>> namespace
>>
>> This is somewhat limited vs the /proc/mounts file, as that calls into
>> vfs hooks through the s_op functions to obtain extra information.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@...aro.org>
>> ---
>>
>>
>> In this patch, I'm interested in your opinions on coding styles.
>> Would you prefer to see the function helpers, (dentry_name, info_opts) where
>> they are, or inside the command as class members? Or perhaps defined in utils?
> 
> Do you think they could be useful beyond this class? If not, stick them
> inside. Refactoring can still be done once they are needed.
> 

Ok - that's good logic. Keep them close until needed elsewhere :)


>>
>> This also shows where I need to take constant information from the kernel.
>> In this case, they are simple numerical bitflags, and unlikely to change but
>> I didn't want to duplicate their values.
>>
>>
>>  scripts/gdb/linux/constants.py.in |  21 ++++++++
>>  scripts/gdb/linux/proc.py         | 110 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 131 insertions(+)
>>
>> diff --git a/scripts/gdb/linux/constants.py.in b/scripts/gdb/linux/constants.py.in
>> index d84084ac945b..739a15d2e984 100644
>> --- a/scripts/gdb/linux/constants.py.in
>> +++ b/scripts/gdb/linux/constants.py.in
>> @@ -12,7 +12,11 @@
>>   *
>>   */
>>  
>> +#include <linux/fs.h>
>> +#include <linux/mount.h>
>> +
>>  /* We need to stringify expanded macros so that they can be parsed */
>>  #define STRING(x) #x
>>  #define XSTRING(x) STRING(x)
>>  
>> @@ -20,3 +24,20 @@
>>  <!-- end-c-headers -->
>>  
>>  import gdb
>> +
>> +/* linux/fs.h */
>> +LX_MS_RDONLY = MS_RDONLY
>> +LX_MS_SYNCHRONOUS = MS_SYNCHRONOUS
>> +LX_MS_MANDLOCK = MS_MANDLOCK
>> +LX_MS_DIRSYNC = MS_DIRSYNC
>> +LX_MS_NOATIME = MS_NOATIME
>> +LX_MS_NODIRATIME = MS_NODIRATIME
>> +
>> +/* linux/mount.h */
>> +LX_MNT_NOSUID = MNT_NOSUID
>> +LX_MNT_NODEV = MNT_NODEV
>> +LX_MNT_NOEXEC = MNT_NOEXEC
>> +LX_MNT_NOATIME = MNT_NOATIME
>> +LX_MNT_NODIRATIME = MNT_NODIRATIME
>> +LX_MNT_RELATIME = MNT_RELATIME
>> +
>> diff --git a/scripts/gdb/linux/proc.py b/scripts/gdb/linux/proc.py
>> index d855b2fd9a06..b79ce2a33a3d 100644
>> --- a/scripts/gdb/linux/proc.py
>> +++ b/scripts/gdb/linux/proc.py
>> @@ -12,6 +12,10 @@
>>  #
>>  
>>  import gdb
>> +from linux import constants
>> +from linux import utils
>> +from linux import tasks
>> +from linux import lists
>>  
>>  
>>  class LxCmdLine(gdb.Command):
>> @@ -96,3 +100,109 @@ Equivalent to cat /proc/ioports on a running target"""
>>          return show_lx_resources("ioport_resource")
>>  
>>  LxIOPorts()
>> +
>> +
>> +# Mount namespace viewer
>> +#  /proc/mounts
>> +
>> +
>> +def dentry_name(d):
>> +    if d['d_parent'] == d:
>> +        return ""
>> +    p = dentry_name(d['d_parent']) + "/"
>> +    return p + d['d_iname'].string()
>> +
>> +
>> +def info_opts(lst, opt):
>> +    opts = ""
>> +    for key, string in lst.items():
>> +        if opt & key:
>> +            opts += string
>> +    return opts
>> +
>> +
>> +FS_INFO = {constants.LX_MS_SYNCHRONOUS: ",sync",
>> +           constants.LX_MS_MANDLOCK: ",mand",
>> +           constants.LX_MS_DIRSYNC: ",dirsync",
>> +           constants.LX_MS_NOATIME: ",noatime",
>> +           constants.LX_MS_NODIRATIME: ",nodiratime"}
>> +
>> +MNT_INFO = {constants.LX_MNT_NOSUID: ",nosuid",
>> +            constants.LX_MNT_NODEV: ",nodev",
>> +            constants.LX_MNT_NOEXEC: ",noexec",
>> +            constants.LX_MNT_NOATIME: ",noatime",
>> +            constants.LX_MNT_NODIRATIME: ",nodiratime",
>> +            constants.LX_MNT_RELATIME: ",relatime"}
>> +
>> +mount_type = utils.CachedType("struct mount")
>> +mount_ptr_type = mount_type.get_type().pointer()
>> +
>> +
>> +class LxMounts(gdb.Command):
>> +    """Report the VFS mounts of the current process namespace.
>> +
>> +Equivalent to cat /proc/mounts on a running target
>> +An integer value can be supplied to display the mount
>> +values of that process namespace"""
>> +
>> +    def __init__(self):
>> +        super(LxMounts, self).__init__("lx-mounts", gdb.COMMAND_DATA)
>> +
>> +    # Equivalent to proc_namespace.c:show_vfsmnt
>> +    # However, that has the ability to call into s_op functions
>> +    # whereas we cannot and must make do with the information we can obtain.
>> +    def invoke(self, arg, from_tty):
>> +        argv = gdb.string_to_argv(arg)
>> +        if len(argv) >= 1:
>> +            try:
>> +                pid = int(argv[0])
>> +            except:
>> +                raise gdb.GdbError("Provide a PID as integer value")
>> +        else:
>> +            pid = 1
>> +
>> +        task = tasks.get_task_by_pid(pid)
>> +        if not task:
>> +            raise gdb.GdbError("Couldn't find a process with PID {}"
>> +                               .format(pid))
>> +
>> +        namespace = task['nsproxy']['mnt_ns']
>> +        if not namespace:
>> +            raise gdb.GdbError("No namespace for current process")
>> +
>> +        for vfs in lists.items(mount_ptr_type, "mnt_list", namespace['list']):
>> +            # There appears to be a null entry at the end of the list...
> 
> "There appears to be" - hmm... Did you check this against the code?


Not properly no... I'll check and see what happened.

I'll have a think as to the best defences to put in.
This one stopped a NULL dereference on mine, but as below clearly it
didn't stop them all!

> 
>> +            if not vfs['mnt_parent']:
>> +                break
>> +
>> +            devname = vfs['mnt_devname'].string()
>> +            devname = devname if devname else "none"
>> +
>> +            pathname = ""
>> +            parent = vfs
>> +            while True:
>> +                mntpoint = parent['mnt_mountpoint']
>> +                pathname = dentry_name(mntpoint) + pathname
> 
> I'm getting an error in this line:
> 
> (gdb) lx-mounts
> devtmpfs /dev devtmpfs rw,relatime 0 0
> tmpfs /dev/shm tmpfs rw,relatime 0 0
> devpts /dev/pts devpts rw,relatime 0 0
> /dev/sda2 / ext4 rw,relatime 0 0
> proc /proc proc rw,nodiratime,relatime 0 0
> sysfs /sys sysfs rw,relatime 0 0
> debugfs /sys/kernel/debug debugfs rw,relatime 0 0
> securityfs /sys/kernel/security securityfs rw,relatime 0 0
> fusectl /sys/fs/fuse/connections fusectl rw,relatime 0 0
> gvfs-fuse-daemon /home/jan/.gvfs fuse rw,relatime,nosuid,nodev 0 0
> tracefs /sys/kernel/debug/tracing tracefs rw,relatime 0 0
> Traceback (most recent call last):
>   File "/data/linux/build-dbg/scripts/gdb/linux/proc.py", line 185, in invoke
>     pathname = dentry_name(mntpoint) + pathname
>   File "/data/linux/build-dbg/scripts/gdb/linux/proc.py", line 112, in dentry_name
>     p = dentry_name(d['d_parent']) + "/"
>   File "/data/linux/build-dbg/scripts/gdb/linux/proc.py", line 110, in dentry_name
>     if d['d_parent'] == d:
> gdb.MemoryError: Cannot access memory at address 0x40
> Error occurred in Python command: Cannot access memory at address 0x40
> 
> In this case, the dump should have stopped after the tracefs line.
> 

Interesting, I'll have to be more defensive.

I'm sure when I looked the proc show command seemed to have only the
list end and the dev name checks...

But we could by our nature run at any time - so it should be a bit more
defensive anyway

>> +                if (parent == parent['mnt_parent']):
>> +                    break
>> +                parent = parent['mnt_parent']
>> +
>> +            if (pathname == ""):
>> +                pathname = "/"
>> +
>> +            superblock = vfs['mnt']['mnt_sb']
>> +            fstype = superblock['s_type']['name'].string()
>> +            s_flags = int(superblock['s_flags'])
>> +            m_flags = int(vfs['mnt']['mnt_flags'])
>> +            rd = "ro" if (s_flags & constants.LX_MS_RDONLY) else "rw"
>> +
>> +            gdb.write(
>> +                "{} {} {} {}{}{} 0 0\n"
>> +                .format(devname,
>> +                        pathname,
>> +                        fstype,
>> +                        rd,
>> +                        info_opts(FS_INFO, s_flags),
>> +                        info_opts(MNT_INFO, m_flags)))
>> +
>> +LxMounts()
>>
> 
> Jan
> 

Kieran

Powered by blists - more mailing lists