netdev - Re: [PATCH bpf v3 2/4] selftest/bpf: Support SOCK_STREAM in unix_inet_redir_to

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: <1612714f-a362-4f42-a05b-53f57b90d6f2@rbox.co>
Date: Wed, 24 Jul 2024 13:36:36 +0200
From: Michal Luczaj <mhal@...x.co>
To: Jakub Sitnicki <jakub@...udflare.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 john.fastabend@...il.com, kuniyu@...zon.com, Rao.Shoaib@...cle.com,
 cong.wang@...edance.com, Andrii Nakryiko <andrii@...nel.org>,
 Eduard Zingerman <eddyz87@...il.com>, Mykola Lysenko <mykolal@...com>
Subject: Re: [PATCH bpf v3 2/4] selftest/bpf: Support SOCK_STREAM in
 unix_inet_redir_to_connected()

On 7/22/24 21:26, Jakub Sitnicki wrote:
> On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:
>>> Classic cleanup with goto to close sockets is all right, but if you're
>>> feeling brave and aim for something less branchy, I've noticed we have
>>> finally started using __attribute__((cleanup)):
>>>
>>> https://elixir.bootlin.com/linux/v6.10/source/tools/testing/selftests/bpf/progs/iters.c#L115
>>
>> I've tried. Is such "ownership passing" (to inhibit the cleanup) via
>> construct like take_fd()[1] welcomed?
> 
> I'm fine with having such a helper to complement the cleanup attribute.
> Alternatively, we can always open code it like it used to be in systemd
> at first [1], if other reviewers don't warm up to it :-)
> 
> [1] https://github.com/systemd/systemd/blob/main/coccinelle/take-fd.cocci

OK, so I've kept create_pair()'s __cleanupfication as the last part of the
series:
https://lore.kernel.org/netdev/20240724-sockmap-selftest-fixes-v1-0-46165d224712@rbox.co

>> [1] https://lore.kernel.org/all/20240627-work-pidfs-v1-1-7e9ab6cc3bb1@kernel.org/
>>
>> static inline void close_fd(int *fd)
>> {
>> 	if (*fd >= 0)
>> 		xclose(*fd);
>> }
>>
>> #define __closefd __attribute__((cleanup(close_fd)))
>>
>> static inline int create_pair(int family, int sotype, int *c, int *p)
>> {
>> 	struct sockaddr_storage addr;
>> 	socklen_t len = sizeof(addr);
>> 	int err;
>>
>> 	int s __closefd = socket_loopback(family, sotype);
>> 	if (s < 0)
>> 		return s;
>>
>> 	err = xgetsockname(s, sockaddr(&addr), &len);
>> 	if (err)
>> 		return err;
>>
>> 	int s0 __closefd = xsocket(family, sotype, 0);
> 
> I'd stick to no declarations in the body. Init to -1 or -EBADF.

All right, it just felt wrong to (demand to) initialize variables with some
magic values. __attribute__((setup(set_negative))) would solve that :) I've
toyed with `DEFINE_CLASS(fd, int, if (_T >= 0) xclose(_T), -EBADF, void)`
but it felt wrong, too.

>> 	case SOCK_STREAM:
>> 	case SOCK_SEQPACKET:
>> 		*p = xaccept_nonblock(s, NULL, NULL);
> 
> I wouldn't touch output arguments until we have succedeed.  Another
> local var will be handy.

OK, sure. Thanks.