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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241211050626.GA128065@j66a10360.sqa.eu95>
Date: Wed, 11 Dec 2024 13:06:26 +0800
From: "D. Wythe" <alibuda@...ux.alibaba.com    >
To: John Ousterhout <ouster@...stanford.edu>
Cc: "D. Wythe" <alibuda@...ux.alibaba.com>, 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 05:59:00PM -0800, John Ousterhout wrote:
> It used to be that RPC ids were only unique to a port, not to a node;
> when I changed Homa to make ids unique within a client node I somehow
> forgot to remove the dport argument from homa_find_server_rpc. I have
> now removed that argument.
> 
> > Anyway, the judgment of whether the skb comes from the same RPC
> > should be consistent, using a unified function or macro definition.
> 
> I agree with this in principle, but this case is a bit special: it's
> on the fast path, and I'd like to not  invoke skb_canonical_ipv6_saddr
> if the ids don't match; if I move this code to a shared function, I'll
> have to pass in the canonical address which will require the
> conversion even when the ids don't match. I don't think there are any
> other places in the code that compare packets for "same RPC" except
> the lookup code in homa_rpc.c, so a shared function would probably
> only be used in this one place. Thus, I'm inclined to leave this code
> as is.

This class maintenance issue is fine as long as the maintainer feels
it's okay. I won't object to this.

> > > +                                     *prev_link = skb2;
> > > +                                     prev_link = &skb2->next;
> > > +                                     continue;
> > > +                             }
> > > +                     }
> > > +                     *other_link = skb2;
> > > +                     other_link = &skb2->next;
> > > +             }
> > > +             *prev_link = NULL;
> > > +             *other_link = NULL;
> > > +             homa_dispatch_pkts(packets, homa);
> 
> > > +
> > > +             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

> 
> Thanks for the explanation; I have now removed the comment.
> 
> > > +     sock_poll_wait(file, sock, wait);
> > > +     mask = POLLOUT | POLLWRNORM;
> > > +
> > > +     if (!list_empty(&homa_sk(sk)->ready_requests) ||
> > > +         !list_empty(&homa_sk(sk)->ready_responses))
> > > +             mask |= POLLIN | POLLRDNORM;
> > > +     return (__poll_t)mask;
> > > +}
> >
> >  Always writable? At least, you should check the state of the
> >  socket. For example, is a socket that has already been shutdown still
> >  writable? Alternatively, how does Homa notify the user when it
> >  receives an ICMP error? You need to carefully consider the return
> >  value of this poll. This is very important for the behavior of the
> >  application.
> 
> I think it's OK for Homa sockets to be writable always. If the socket
> has been shut down and an application attempts to write to it, it will
> get ESHUTDOWN then. Message sends in Homa are always asynchronous, so
> there is no notion of them blocking. ICMP errors are reflected at the
> level of RPCs (on the server side, the RPC is discarded if an ICMP
> error occurs; on the client side, an error will be returned as
> response, which will make the socket "readable").

Sounds reasonable.

> 
> 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.

> 
> -John-