Re: [PATCH v1 09/10] gdb/linux-tdep: parse ProtectionKey in /proc/PID/smaps

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Matthieu Longo <matthieu.longo@arm.com> writes:

> Memory Protection Keys provide a mechanism for enforcing page-based
> protections without requiring modification of the page tables
> when an application changes protection domains. [1]
>
> The "ProtectionKey" field may be present in /proc/PID/smaps x86_64
> and AArch64 systems since Linux 4.9, when the kernel is built with
> Memory Protection Keys support.

/me from a previous life would feel the need to point out that it may
also be present on POWER systems. :)

> Add a new 'pkey' field to `struct smaps_data', and populate it when
> the "ProtectionKey" field is present.
>
> This prepares for displaying the protection key in 'info proc mappings'.
>
> [1]: https://docs.kernel.org/core-api/protection-keys.html,
>      https://lkml.iu.edu/1512.0/03058.html

I suggest linking to the commit that went upstream instead of the
mailing list patch:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c1192f842841

> ---
>  gdb/linux-tdep.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index dbc69a5a7fc..d89c5ae8504 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -124,6 +124,7 @@ struct smaps_data
>  
>    ULONGEST rss;
>    ULONGEST swap;
> +  std::optional<int> pkey;

There's nothing in this patch series that uses the parsed pkey, so IMHO
(other maintainers may disagree) it makes more sense if this patch is
committed together with a patch that makes use of the field.

>  };
>  
>  /* Whether to take the /proc/PID/coredump_filter into account when
> @@ -1553,6 +1554,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>        int mapping_file_p;
>        ULONGEST rss = -1;
>        ULONGEST swap = -1;
> +      int pkey = -1;
>  
>        memset (&v, 0, sizeof (v));
>        struct mapping m = read_mapping (line);
> @@ -1653,6 +1655,16 @@ parse_smaps_data (const file_reader_t<char> &freader)
>  		  mapping_anon_p = 1;
>  		}
>  	    }
> +
> +	  if (streq (keyword, "ProtectionKey:"))
> +	    {
> +	      if (sscanf (line, "%*s%d", &pkey) != 1)
> +		{
> +		  warning (_("Error parsing %s's value in {s,}maps file '%s'"),
> +			   keyword, freader.c_filepath ());
> +		  break;
> +		}
> +	    }
>  	}
>        /* Save the smaps entry to the vector.  */
>  	struct smaps_data map;
> @@ -1672,6 +1684,7 @@ parse_smaps_data (const file_reader_t<char> &freader)
>  	map.inode = m.inode;
>  	map.rss = rss;
>  	map.swap = swap;
> +	map.pkey.emplace (pkey);

Shouldn't map.pkey be set only if pkey != -1?

>  
>  	smaps.emplace_back (map);
>      }

-- 
Thiago
(he/him)