linux-kernel - Re: [PATCH] netclassid: on modifying netclassid, only consider the main 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-prev] [day] [month] [year] [list]

Message-ID: <ZSQYyO9JW/9HEjPM@kernel.org>
Date:   Mon, 9 Oct 2023 17:14:16 +0200
From:   Simon Horman <horms@...nel.org>
To:     zhailiansen <zhailiansen@...il.com>
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, yuwang@...ishou.com,
        wushukun@...ishou.com, zhailiansen <zhailiansen@...ishou.com>
Subject: Re: [PATCH] netclassid: on modifying netclassid, only consider the
 main process.

On Sun, Oct 08, 2023 at 11:04:42AM +0800, zhailiansen wrote:

Hi,

thanks for your patch.
Some minor comments from my side.

> When modifying netclassid, the command("echo 0x100001 > net_cls.classid") will
> take more time on muti threads of one process, because the process create many

nit: muti -> multiple ?

> fds.
> for example, one process exists 28000 fds and 60000 threads, echo command will
> task 45 seconds.
> Now, we only consider the main process when exec "iterate_fd", and the time is
> 52 milliseconds.

Please consider line-wrapping the patch description at 75 bytes.

This patch seems best targeted at net-next, this information should be
included in the patch. So if post a v2 please consider using:

	Subject: [PATCH v2] ...

> Signed-off-by: zhailiansen <zhailiansen@...ishou.com>

Please consider signing off using your real name.

e.g. Signed-off-by: Firstname Lastname <...>

> ---
>  net/core/netclassid_cgroup.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/core/netclassid_cgroup.c b/net/core/netclassid_cgroup.c
> index d6a70ae..78c903c 100644
> --- a/net/core/netclassid_cgroup.c
> +++ b/net/core/netclassid_cgroup.c
> @@ -88,6 +88,12 @@ static void update_classid_task(struct task_struct *p, u32 classid)
>  	};
>  	unsigned int fd = 0;
>  
> +	/* Only update the leader task, when multi threads in this task,
> +	 * so it can avoid the useless traversal.
> +	 */
> +	if (p && p != p->group_leader)
> +		return;

Can p ever be NULL?
It is dereferenced unconditionally by the existing call to task_lock() below.

> +
>  	do {
>  		task_lock(p);
>  		fd = iterate_fd(p->files, fd, update_classid_sock, &ctx);
> -- 
> 1.8.3.1
> 
>