On Fri, Oct 1, 2021 at 12:01 AM Rasmus Villemoes
<linux@xxxxxxxxxxxxxxxxxx> wrote:
>
> On 03/09/2021 01.18, Suren Baghdasaryan wrote:
> > From: Colin Cross <ccross@xxxxxxxxxx>
> >
> >
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx@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.
>
>
> > +/* 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.
