On Fri, Oct 1, 2021 at 12:01 AM Rasmus Villemoes
[off-list ref] wrote:

On 03/09/2021 01.18, Suren Baghdasaryan wrote:

quoted

From: Colin Cross <redacted>


changes in v9
- Changed max anon vma name length from 64 to 256 (as in the original patch)
because I found one case of the name length being 139 bytes. If anyone is
curious, here it is:
dalvik-/data/dalvik-cache/arm64/apex@com.android.permission@priv-app@GooglePermissionController@GooglePermissionController.apk@classes.art

I'm not sure that's a very convincing argument. We don't add code
arbitrarily just because some userspace code running on some custom
kernel (ab)uses something in that kernel. Surely that user can come up
with a name that doesn't contain GooglePermissionController twice.

The argument for using strings and not just a 128 bit uuid was that it
should (also) be human readable, and 250-byte strings are not that.
Also, there's no natural law forcing this to be some power-of-two, and
in fact the implementation means that it's actually somewhat harmful
(give it a 256 char name, and we'll do a 260 byte alloc, which becomes a
512 byte alloc). So just make the limit 80, the kernel's definition of a
sane line length.

Sounds reasonable. I'll set the limit to 80 and will look into the
userspace part if we can trim the names to abide by this limit.

As for the allowed chars, it can be relaxed later if convincing arguments can be made.

For the disallowed chars, I would like to go with "\\`$[]" set because
of the example I presented in my last reply. Since we disallow $, the
parsers should be able to process parentheses with no issues I think.

quoted

+/* mmap_lock should be read-locked */
+static inline bool is_same_vma_anon_name(struct vm_area_struct *vma,
+                                      const char *name)
+{
+     const char *vma_name = vma_anon_name(vma);
+
+     if (likely(!vma_name))
+             return name == NULL;
+
+     return name && !strcmp(name, vma_name);

It's probably preferable to spell this

  /* either both NULL, or pointers to same refcounted string */
  if (vma_name == name)
      return true;

  return name && vma_name && !strcmp(name, vma_name);

so you have one less conditional in the common case.

Ack.

Rasmus

Thanks for the review!
Suren.