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 - 1.34
+++ if_pflow.c 30 Aug 2013 18:54:03 -
@@ -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.
