On Mon, Sep 02, 2013 at 11:11:43AM +0200, Martin Pieuchot wrote:
> On 31/08/13(Sat) 04:28, Nathanael Rensen wrote:
> > If no flowsrc is specified on a pflow(4) interface then the src address
> > is determined by ip_output(). However prior to calling ip_output() pflow(4)
> > has already calculated the UPD pseudo-header checksum based on INADDR_ANY.
> > This results in a bad UPD checksum and the resulting pflow packet is
> > rejected by the receiver.
> >
> > The diff below resolves this by calling in_selectsrc() if flowsrc is not
> > specified.
>
> I'm not sure we want to add yet-another different chunk of code to
> determine a source address, especially when I'm working on reducing
> their number ;)
>
> I'm not familiar with pflow(4), but is there any advantage of not
> specifying a flowsrc target? Because reducing the number of code
> path passing an INADDR_ANY source address would simplify a lot of
> our address lookups.
If there is no strong usecase for not specifying flowsrc I think it's
better to not take the interface up if there is no flowsrc like we are
doing with flowdst. That requires some dicking around in pflowioctl()
and needs to be documented. (And don't take it up if someone sets it
explicitly to INADDR_ANY.)
Also style(9) says:
Don't put declarations inside
blocks unless the routine is unusually complicated.
>
> Otherwise did you considered deferring the checksum?
>
> > Index: if_pflow.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/if_pflow.c,v
> > retrieving revision 1.34
> > diff -u -p -r1.34 if_pflow.c
> > --- if_pflow.c 13 Aug 2013 08:44:05 -0000 1.34
> > +++ if_pflow.c 30 Aug 2013 18:54:03 -0000
> > @@ -1512,8 +1512,37 @@ pflow_sendout_mbuf(struct pflow_softc *s
> > struct ifnet *ifp = &sc->sc_if;
> > #endif
> > struct ip *ip;
> > + struct in_addr sender_ip;
> > int err;
> >
> > + /* Determine the sender address */
> > + sender_ip = sc->sc_sender_ip;
> > + if (sender_ip.s_addr == INADDR_ANY) {
> > + struct sockaddr_in sin;
> > + struct sockaddr_in *ifaddr;
> > + struct route ro;
> > +
> > + bzero(&ro, sizeof(ro));
> > + bzero(&sin, sizeof(sin));
> > + sin.sin_len = sizeof(sin);
> > + sin.sin_family = AF_INET;
> > + sin.sin_port = sc->sc_receiver_port;
> > + sin.sin_addr = sc->sc_receiver_ip;
> > +
> > + err = 0;
> > + ifaddr = in_selectsrc(&sin, &ro, 0, NULL, &err, 0);
> > + if (ifaddr == NULL) {
> > + if (err == 0)
> > + err = EADDRNOTAVAIL;
> > + return err;
> > + }
> > +
> > + sender_ip = ifaddr->sin_addr;
> > +
> > + if (ro.ro_rt)
> > + rtfree(ro.ro_rt);
> > + }
> > +
> > /* UDP Header*/
> > M_PREPEND(m, sizeof(struct udpiphdr), M_DONTWAIT);
> > if (m == NULL) {
> > @@ -1523,7 +1552,7 @@ pflow_sendout_mbuf(struct pflow_softc *s
> >
> > ui = mtod(m, struct udpiphdr *);
> > ui->ui_pr = IPPROTO_UDP;
> > - ui->ui_src = sc->sc_sender_ip;
> > + ui->ui_src = sender_ip;
> > ui->ui_sport = sc->sc_sender_port;
> > ui->ui_dst = sc->sc_receiver_ip;
> > ui->ui_dport = sc->sc_receiver_port;
> >
>
--
I'm not entirely sure you are real.
