netdev - Re: [PATCH net-next v3 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: <CAGXJAmwVyzS34tP5xuK8p_=MM+E06EKuM5NVwd7a84e97i7tsA@mail.gmail.com>
Date: Wed, 11 Dec 2024 14:59:05 -0800
From: John Ousterhout <ouster@...stanford.edu>
To: "D. Wythe" <alibuda@...ux.alibaba.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 11/12] net: homa: create homa_plumbing.c homa_utils.c

On Tue, Dec 10, 2024 at 9:06 PM D. Wythe <alibuda@...ux.alibaba.com> wrote:
>
> > > > +
> > > > +             iph = (struct iphdr *)(icmp + sizeof(struct icmphdr));
> > > > +             h = (struct common_header *)(icmp + sizeof(struct icmphdr)
> > > > +                             + iph->ihl * 4);
> > >
> > > You need to ensure that the comm_header can be accessed linearly. The
> > > icmp_socket_deliver() only guarantees that the full IP header plus 8 bytes
> > > can be accessed linearly.
> >
> > This code only accesses the destination port, which is within the
> > first 4 bytes of the common_header (the same position as in a TCP
> > header). Thus I think it's safe without calling pskb_may_pull?
>
> You are right, but there is still a small issue. The standard
> practice is to directly obtain the nested iphdr from skb->data, rather than
> from icmphdr + size.
>
> FYI:
>                 <  skb->head
> ...
> iphdr           <- head + network_header
> icmphdr         <- head + transport_header
> nested-iphdr    <- skb->data
> homahdr         <- skb->data + iph->ihl * 4

Ahah, I wasn't aware of this practice, but it makes total sense; I've
modified the code as you suggested.

> > However, your comment makes me wonder about polling for read on a
> > socket that has been shutdown. If I don't return -ESHUTDOWN from
> > homa_poll in this case, I believe the application could block waiting
> > for reads on a socket that has been shutdown? Of course this will
> > never complete. So, should I check for shutdown and return -ESHUTDOWN
> > immediately in homa_poll, before invoking sock_poll_wait?
>
> Always invoke sock_poll_wait before checking any state, If you got RCV_SHUTDOWN,
> you should return at least a mask with POLLIN (not -SHUTDOWN), then
> return 0 (in most cases) in homa_recvmsg(). You can refer to tcp_poll for
> this.

Done.

-John-