Re: [PATCH 1/2] Implement support for checking /proc/PID/coredump_filter

[Pedro Alves <palves at redhat dot com>]

Thanks, this looks good to me now, just a few minor nits.

On 03/19/2015 11:22 PM, Sergio Durigan Junior wrote:

> +/* Helper function to decode the "VmFlags" field in /proc/PID/smaps.
> +
> +   This function was based on the documentation found on
> +   <Documentation/filesystems/proc.txt>, on the Linux kernel.
> +
> +   Linux kernels before commit
> +   834f82e2aa9a8ede94b17b656329f850c1471514 do not have this field on
> +   smaps.  */

Thanks for the version info.  Ideally, what I'd like with these version notes
is at any point in the time in the future being able to look at the code
and easily identify whether we're working around an old enough kernel that
we potentially can stop worrying about backwards compatibility with
such kernel.  If easy, could you also add the kernel version, like
e.g., "$hash (x.y.z)"?

> +  if (init_regex_p == -1)
> +    {
> +      /* There was an error while compiling the regex'es above.  In
> +	 order to try to give some reliable information to the caller,
> +	 we just try to find the string "(deleted)" in the filename.
> +	 If we managed to find it, then we assume the mapping is
> +	 anonymous.  */
> +      return strstr (filename, "(deleted)") != NULL;

Might as well add the space before parens, and match tail
end only, like e.g.,:

   const char deleted[] = " (deleted)";
   size_t del_len = strlen (deleted);
   size_t filename_len = strlen (filename);

   return (filename_len >= del_len
           && strcmp (filename + filename_len - del_len, deleted) == 0);

> +
> +/* Return 0 if the memory mapping (which is related to FILTERFLAGS, V,
> +   MAYBE_PRIVATE_P, and MAPPING_ANONYMOUS_P) should not be dumped, or
> +   greater than 0 if it should.
> +
> +   In a nutshell, this is the logic that we follow in order to decide
> +   if a mapping should be dumped or not.
> +
> +   - If the mapping is associated to a file whose name ends with "
> +     (deleted)"

Nit: it'd be good to avoid breaking " (deleted)" over two lines.

        > , or if the file is "/dev/zero", or if it is
> +     "/SYSV%08x" (shared memory), or if there is no file associated
> +     with it, or if the AnonHugePages: or the Anonymous: fields in the
> +     /proc/PID/smaps have contents, then GDB considers this mapping to
> +     be anonymous.  Otherwise, GDB considers this mapping to be a
> +     file-backed mapping (because there will be a file associated with
> +     it).
> + 
> +     It is worth mentioning that, from all those checks described
> +     above, the most fragile is the one to see if the file name ends
> +     with " (deleted)".  This does not necessarily mean that the
> +     mapping is anonymous, because the deleted file associated with
> +     the mapping may have been a hard link to another file, for
> +     example.  The Linux kernel checks to see if "i_nlink == 0", but
> +     GDB cannot easily do this check.  Therefore, we made a compromise

Cannot do it easily, or cannot do it at all?

> +     here, and we assume that if the file name ends with " (deleted)",
> +     then the mapping is indeed anonymous.  FWIW, this is something
> +     the Linux kernel could do better: expose this information in a
> +     more direct way.
> + 
> +   - If we see the flag "sh" in the "VmFlags:" field (in
> +     /proc/PID/smaps), then certainly the memory mapping is shared
> +     (VM_SHARED).  If we have access to the VmFlags, and we don't see
> +     the "sh" there, then certainly the mapping is private.  However,
> +     Linux kernels before commit
> +     834f82e2aa9a8ede94b17b656329f850c1471514 do not have the
> +     "VmFlags:" field; in that case, we use another heuristic: if we
> +     see 'p' in the permission flags, then we assume that the mapping
> +     is private, even though the presence of the 's' flag there would
> +     mean VM_MAYSHARE, which means the mapping could still be private.
> +     This should work OK enough, however.  */
> +
> +static int
> +dump_mapping_p (unsigned int filterflags, const struct smaps_vmflags *v,
> +		int maybe_private_p, int mapping_anon_p, const char *filename)
> +{
> +  /* Initially, we trust in what we received from outside.  This value

outside == the caller?  or the kernel?  The former, right?

> +     may not be very precise (i.e., it was probably gathered from the
> +     permission line in the /proc/PID/smaps list, which actually
> +     refers to VM_MAYSHARE, and not VM_SHARED), but it is what we have
> +     for now.  */

"for now" refers to the flag being updated further down, or future
kernel versions?

> +  int private_p = maybe_private_p;
> +
> +  /* We always dump vDSO and vsyscall mappings.  */

Suggest:

   /* We always dump vDSO and vsyscall mappings, because it's likely that
      there'll be no file to read the contents from at core load time.
      The kernel does the same.  */

Thanks,
Pedro Alves