Hi Pablo, On Thu, Dec 13, 2018 at 2:28 AM Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote: > > Hi Alin, > > On Wed, Dec 12, 2018 at 05:56:46PM +0100, Alin Năstac wrote: > [...] > > > One thing we could probably do is to add nf_nat_rtp_rtcp_expected() > > > instead of using the generic nf_nat_sip_expected() which is also > > > called for the REGISTER case. I think we only need this code below for > > > the RTP/RTCP case as you explained. > > > > That's true, but signalling expectations do not have a paired > > expectation anyway. > > If you want to exclude this logic for them, it is enough to condition > > it with exp->helper != nfct_help(ct)->helper. > > > > What variant do you prefer? > > Restricting the helper should be fine too, so we don't need a new > function. Probably you can check for the expectation class instead? > > > > > > > > + range.flags = (NF_NAT_RANGE_MAP_IPS | NF_NAT_RANGE_PROTO_SPECIFIED); > > > > > > > + range.min_proto.all = range.max_proto.all = pair_exp->tuple.dst.u.all; > > > > > > > + range.min_addr = range.max_addr = pair_exp->tuple.dst.u3; > > > > > > > + range_set_for_snat = 1; > > I can see in the existing code: > > #1 do DST manip case: so range.flags, .min_proto and .min_addr are set. > #2 In case of paired expectation in place, we overwrite these range fields > again. > #3 No override for the _SRC case. > > If your patch can help disentagle this code a bit by: > > #1 check for paired expectation, if so do handling, return. > #2 check for _SRC manip needed, return. > #3 existing _DST manip case. DST manip handling is needed at least in one RTP case: on the expectation created by the INVITE send from LAN. So you see, I can't return from this function just because one manip logic applies, doing so will break the case where the applied manip is equivalent to no operation. > It may be larger patch, but we skip 'range_set_for_snat'? What is wrong with using booleans? If you want, I can use range.flags for the same purpose (I would have to reset it to 0 after doing DST manip), but I thought this would make code reading more difficult. Besides, if I would use return in the loop I would have to duplicate the spin unlocking within the loop and this will look ugly. > More comments below. > > > > > > > > + break; > > > > > > > + } > > > > > > > + } > > > > > > > + spin_unlock_bh(&nf_conntrack_expect_lock); > > > > > > > + > > > > > > > + /* When no paired expected conntrack has been found, change src to > > > > > > > + * where master sends to, but only if the connection actually came > > > > > > > + * from the same source. */ > > For new comments, we prefer this format: > > /* When no paired expected conntrack has been found, change src to > * where master sends to, but only if the connection actually came > * from the same source. > */ > > Even if many old spots still use the old format. > > And copy and paste the long description you made in your previous > email in the commit log, it's good for the record :-). > > Thanks a lot for your patience !
