netdev - Re: [PATCH net-next v2 11/12] net: homa: create homa_plumbing.c homa

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: <CAGXJAmysiRx3VNDxXitPOn+yg2ck_+7fbd1XSsQQqZnwUEkvPQ@mail.gmail.com>
Date: Wed, 4 Dec 2024 11:54:59 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: "D. Wythe" <alibuda@...ux.alibaba.com>
Cc: netdev@...r.kernel.org, linux-api@...r.kernel.org
Subject: Re: [PATCH net-next v2 11/12] net: homa: create homa_plumbing.c homa_utils.c

On Mon, Nov 25, 2024 at 9:32 PM D. Wythe <alibuda@...ux.alibaba.com> wrote:
> On 11/12/24 7:40 AM, John Ousterhout wrote:
> > +static struct proto homav6_prot = {
> > ...
> > +     .obj_size          = sizeof(struct homa_sock) + sizeof(struct ipv6_pinfo),
>
> The implementation of inet6_sk_generic() has already changed, you should set
> .ipv6_pinfo_offset.

Fixed.

> > +static int __init homa_load(void)
> > ...
> > +     inet_register_protosw(&homa_protosw);
> > +     inet6_register_protosw(&homav6_protosw);
>
>
> better to check the retval of inet6_register_protosw().

Fixed.

> > +out_cleanup:
> > +     homa_destroy(homa);
> > +     inet_del_protocol(&homa_protocol, IPPROTO_HOMA);
> > +     inet_unregister_protosw(&homa_protosw);
> > +     inet6_del_protocol(&homav6_protocol, IPPROTO_HOMA);
> > +     inet6_unregister_protosw(&homav6_protosw);
> > +     proto_unregister(&homa_prot);
> > +     proto_unregister(&homav6_prot);
>
>
> It's a bit strange for me that this relies on a premise: that every reverse operation can correctly
> identify whether the corresponding forward operation has been executed. Currently, perhaps every
> function includes this capability. It's up to you, I don't insist.

Actually, not all of the cleanup functions are safe if the initializer
hasn't been invoked; good catch. I've fixed this now.

> Perhaps you can try adding MODULE_ALIAS_NET_PF_PROTO_TYPE so that the kernel will automatically load
> the module when creating IPPROTO_HOMA socket. A functional suggestion, It's up to you.

Done; thanks for the suggestion (I wasn't aware of this feature).

> Is binding multiple times legal? For example, bind 80 first and then bind 8080. If not, I think
> you might need to check the inet_num.

Yes, it's legal.

> I noticed that homa_sock_init() contains a memory allocation action, perhaps you should add a return
> value check.

Oops; I've fixed this now.

Thanks for all the comments.

-John-