netdev - Re: [patch net-next v3 5/9] genetlink: implement release callback and free sk_user

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231123082408.0c038f30@kernel.org>
Date: Thu, 23 Nov 2023 08:24:08 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Jiri Pirko <jiri@...nulli.us>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, davem@...emloft.net,
 edumazet@...gle.com, jacob.e.keller@...el.com, jhs@...atatu.com,
 johannes@...solutions.net, andriy.shevchenko@...ux.intel.com,
 amritha.nambiar@...el.com, sdf@...gle.com, horms@...nel.org
Subject: Re: [patch net-next v3 5/9] genetlink: implement release callback
 and free sk_user_data there

On Thu, 23 Nov 2023 11:32:07 +0100 Jiri Pirko wrote:
> In this case, the socket is not opened by kernel, but it is opened by
> the userspace app.
> 
> I basically need to have per-user-sk pointer somewhere I'm not clear why
> to put it in struct netlink_sock when I can use sk_user_data which is
> already there. From the usage of this pointer in kernel, I understand
> this is exactly the reason to have it.

Various people stuck various things in that pointer just because,
it's a mess. IIUC the initial motivation for it is that someone
like NFS opens a kernel socket and needs to put private data
somewhere. A kernel user gets a callback for a socket, like data
ready, and needs to find their private state.

> Are you afraid of a collision of sk_user_data use with somebody else
> here? I don't see how that could happen for netlink socket.

Normally upper layer wraps the socket struct in its own struct. 
Look at the struct nesting for TCP or any other bona fide protocol. 
genetlink will benefit from having socket state, I bet it wasn't done
that way from the start because Jamal/Thomas were told to start small.

Please add a properly typed field to the netlink struct, unless you
have technical reasons not to.