linux-kernel - Re: [PATCH 1/3] ksm: add ksm involvement information for each process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240903042754.1776245-1-xu.xin16@zte.com.cn>
Date: Tue,  3 Sep 2024 04:27:54 +0000
From: xu xin <xu.xin.sc@...il.com>
To: david@...hat.com
Cc: linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org,
	shr@...kernel.io,
	hughd@...gle.com,
	xu.xin16@....com.cn
Subject: Re: [PATCH 1/3] ksm: add ksm involvement information for each process

> > In /proc/<pid>/ksm_stat, Add two extra ksm involvement items including
> > KSM_mergeable and KSM_merge_any. It helps administrators to
> > better know the system's KSM behavior at process level.
> > 
> > KSM_mergeable: yes/no
> >     whether any VMAs of the process'mm are currently applicable to KSM.
> > 
> > KSM_merge_any: yes/no
> >     whether the process'mm is added by prctl() into the candidate list
> >     of KSM or not, and fully enabled at process level.
> > 
> > Signed-off-by: xu xin <xu.xin16@....com.cn>
> > ---
> > Changelog
> > =========
> > v2 -> v3:
> >          Update the KSM_mergeable getting method: loop up if any vma is
> >          mergeable to KSM.
> >         https://lore.kernel.org/all/bc0e1cdd-2d9d-437c-8fc9-4df0e13c48c0@redhat.com/
> > 
> > v1 -> v2:
> >          replace the internal flag names with straightforward strings.
> >          * MMF_VM_MERGEABLE -> KSM_mergeable
> >          * MMF_VM_MERGE_ANY -> KSM_merge_any
> > 
> >   fs/proc/base.c      |  4 ++++
> >   include/linux/ksm.h |  1 +
> >   mm/ksm.c            | 16 ++++++++++++++++
> >   3 files changed, 21 insertions(+)
> > 
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 18550c071d71..45e12560e698 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -3217,6 +3217,10 @@ static int proc_pid_ksm_stat(struct seq_file *m, struct pid_namespace *ns,
> >           seq_printf(m, "ksm_zero_pages %lu\n", mm->ksm_zero_pages);
> >           seq_printf(m, "ksm_merging_pages %lu\n", mm->ksm_merging_pages);
> >           seq_printf(m, "ksm_process_profit %ld\n", ksm_process_profit(mm));
> > +        seq_printf(m, "KSM_mergeable: %s\n",
> > +                ksm_process_mergeable(mm) ? "yes" : "no");
> > +        seq_printf(m, "KSM_merge_any: %s\n",
> > +                test_bit(MMF_VM_MERGE_ANY, &mm->flags) ? "yes" : "no");
> 
> Inconsistent "KSM" casing .

Excuse me, could you be more specific? I didn't get it
> 
> >           mmput(mm);
> >       }
> >   
> > diff --git a/include/linux/ksm.h b/include/linux/ksm.h
> > index 52c63a9c5a9c..5286b84964d3 100644
> > --- a/include/linux/ksm.h
> > +++ b/include/linux/ksm.h
> > @@ -84,6 +84,7 @@ void folio_migrate_ksm(struct folio *newfolio, struct folio *folio);
> >   void collect_procs_ksm(struct folio *folio, struct page *page,
> >           struct list_head *to_kill, int force_early);
> >   long ksm_process_profit(struct mm_struct *);
> > +bool ksm_process_mergeable(struct mm_struct *mm);
> >   
> >   #else  /* !CONFIG_KSM */
> >   
> > diff --git a/mm/ksm.c b/mm/ksm.c
> > index f5138f43f0d2..6647f2ef27ca 100644
> > --- a/mm/ksm.c
> > +++ b/mm/ksm.c
> > @@ -3373,6 +3373,22 @@ static void wait_while_offlining(void)
> >   #endif /* CONFIG_MEMORY_HOTREMOVE */
> >   
> >   #ifdef CONFIG_PROC_FS
> > +/*
> > + * The process is mergeable only if any VMA (and which) is currently
> 
> confusing "(and which)"

Oh yes, I'll correct it

> 
> > + * applicable to KSM.
> > + */
> > +bool ksm_process_mergeable(struct mm_struct *mm)
> > +{
> > +    struct vm_area_struct *vma;
> > +
> > +    VMA_ITERATOR(vmi, mm, 0);
> > +    for_each_vma(vmi, vma)
> > +        if (vma->vm_flags & VM_MERGEABLE)
> > +            return true;
> > +
> 
> Are we holding the mmap lock here? I only see a mmput() in 
> proc_pid_ksm_stat() above, which might mean that we are not holding the 
> mmap lock.

No, we'are missing the read lock of mmap. I'll fix it.
Thanks a lot.