[Crash-utility] [PATCH] fix command vm/files -d/mount on new kernel

[Dave Anderson]

----- Original Message -----
> At 2012-3-10 5:50, Dave Anderson wrote:
> >
> > It's failing in its call to get_pathname().  I haven't had a chance to
> 
> You are right. I forgot to check every call to get_pathname. The new
> patch has fixed the problem of swap.

Although the patch appears to work in the simple cases that I have tested,
I have a few issues with it.

First, the get_pathname() function prototype is -- and has always been -- this:

  void get_pathname(ulong dentry, char *pathname, int length, int full, ulong vfsmnt)

where the "vfsmnt" argument has always been a struct vfsmount pointer.

With your patch, prior to making get_pathname() calls, you typically
make a VALID_STRUCT(mount) call first, and if true, you modify the
vfsmnt argument to be a struct mount pointer.  

It would make more sense maintenance-wise, and would simplify the patch,
if the get_pathname() "vfsmnt" argument would continue to be a struct 
vfsmount pointer.  And at the top of get_pathname(), you could make the
adjustments for when it is embedded inside a struct mount.

For example, taking these patches to open_files_dump() and file_dump():

@@ -2226,12 +2299,16 @@ open_files_dump(ulong task, int flags, struct reference *ref)
                        if (VALID_MEMBER(fs_struct_rootmnt)) {
                                vfsmnt = ULONG(fs_struct_buf +
                                        OFFSET(fs_struct_rootmnt));
+                               if (VALID_STRUCT(mount))
+                                       vfsmnt = vfsmnt - OFFSET(mount_mnt);
                                get_pathname(root_dentry, root_pathname,
                                        BUFSIZE, 1, vfsmnt);
                        } else if (use_path) {
                                vfsmnt = ULONG(fs_struct_buf +
                                        OFFSET(fs_struct_root) +
                                        OFFSET(path_mnt));
+                               if (VALID_STRUCT(mount))
+                                       vfsmnt = vfsmnt - OFFSET(mount_mnt);
                                get_pathname(root_dentry, root_pathname,
                                        BUFSIZE, 1, vfsmnt);
                        } else {
@@ -2250,12 +2327,16 @@ open_files_dump(ulong task, int flags, struct reference *ref)
                        if (VALID_MEMBER(fs_struct_pwdmnt)) {
                                vfsmnt = ULONG(fs_struct_buf +
                                        OFFSET(fs_struct_pwdmnt));
+                               if (VALID_STRUCT(mount))
+                                       vfsmnt = vfsmnt - OFFSET(mount_mnt);
                                get_pathname(pwd_dentry, pwd_pathname,
                                        BUFSIZE, 1, vfsmnt);
                        } else if (use_path) {
                                vfsmnt = ULONG(fs_struct_buf +
                                        OFFSET(fs_struct_pwd) +
                                        OFFSET(path_mnt));
+                               if (VALID_STRUCT(mount))
+                                       vfsmnt = vfsmnt - OFFSET(mount_mnt);
                                get_pathname(pwd_dentry, pwd_pathname,
                                        BUFSIZE, 1, vfsmnt);
@@ -2686,7 +2767,12 @@ file_dump(ulong file, ulong dentry, ulong inode, int fd, int flags)
        if (flags & DUMP_FULL_NAME) {
                if (VALID_MEMBER(file_f_vfsmnt)) {
                        vfsmnt = get_root_vfsmount(file_buf);
-                       get_pathname(dentry, pathname, BUFSIZE, 1, vfsmnt);
+                       if (VALID_STRUCT(mount))
+                               get_pathname(dentry, pathname, BUFSIZE, 1,
+                                               vfsmnt - OFFSET(mount_mnt));
+                       else
+                               get_pathname(dentry, pathname, BUFSIZE, 1,
+                                               vfsmnt);
                        if (STRNEQ(pathname, "/pts/") &&
                            STREQ(vfsmount_devname(vfsmnt, buf1, BUFSIZE),
                            "devpts"))

If the vfsmount-to-mount calculation were always done in get_pathname() itself,
then the patches above would be unnecessary.  The same is true for a few other
parts of the patch.

Did you consider doing it that way? 

And consider the following patched version of display_dentry_info() below.
The two get_pathname() calls below would generate fatal errors on older 
kernels because OFFSET(mount_mnt) will be invalid:

                if (!found && symbol_exists("pipe_mnt")) {
                        get_symbol_data("pipe_mnt", sizeof(long), &vfs);
                        if (VALID_STRUCT(mount))
                                readmem(vfs - OFFSET(mount_mnt), KVADDR, mount_buf, SIZE(mount),
                                        "mount buffer", FAULT_ON_ERROR);
                        else
                                readmem(vfs, KVADDR, vfsmount_buf, SIZE(vfsmount),
                                        "vfsmount buffer", FAULT_ON_ERROR);
                        sb = ULONG(vfsmount_buf + OFFSET(vfsmount_mnt_sb));
                        if (superblock && (sb == superblock)) {
                                get_pathname(dentry, pathname, BUFSIZE, 1,
                                                vfs - OFFSET(mount_mnt));
                                found = TRUE;
                        }
                }
                if (!found && symbol_exists("sock_mnt")) {
                        get_symbol_data("sock_mnt", sizeof(long), &vfs);
                        if (VALID_STRUCT(mount))
                                readmem(vfs - OFFSET(mount_mnt), KVADDR, mount_buf, SIZE(mount),
                                        "mount buffer", FAULT_ON_ERROR);
                        else
                                readmem(vfs, KVADDR, vfsmount_buf, SIZE(vfsmount),
                                        "vfsmount buffer", FAULT_ON_ERROR);
                        sb = ULONG(vfsmount_buf + OFFSET(vfsmount_mnt_sb));
                        if (superblock && (sb == superblock)) {
                                get_pathname(dentry, pathname, BUFSIZE, 1,
                                                vfs - OFFSET(mount_mnt));
                                found = TRUE;
                        }
                }

If we can continue to have get_pathname() receive a vfsmount pointer,
it reduces the chance of introducing new bugs like the above, and future
maintenance will be easier.

Comments?
 
Dave