Re: pf: broken RB comparator drops IPv6 fragments

On Fri, Apr 10, 2026 at 11:20:34AM +0200, Otto Moerbeek wrote:
> From: Otto Moerbeek <[email protected]>
> To: Alexandr Nedvedicky <[email protected]>
> Cc: Renaud Allard <[email protected]>, [email protected]
> Subject: Re: pf: broken RB comparator drops IPv6 fragments
> Date: Fri, 10 Apr 2026 11:20:34 +0200
> 
> On Fri, Apr 10, 2026 at 10:41:32AM +0200, Alexandr Nedvedicky wrote:
> 
> > Hello,
> > 
> > On Fri, Apr 10, 2026 at 08:56:33AM +0200, Renaud Allard wrote:
> > > pf_frag_compare() in sys/net/pf_norm.c uses subtraction to compare
> > > u_int32_t fragment IDs:
> > > 
> > > 	if ((diff = a->fr_id - b->fr_id) != 0)
> > > 		return (diff);
> > > 
> > 
> > </snip>
> > 
> >     I wonder if diff below would work too here...
> 
> Maybe, but it's hard to reason about that as it requires intimate
> knowledge of the (implicit) type conversion rules. I much prefer
> writing it out in a clear way as Renaud did. See also the comment in
> qsort(3).
> 
> 	-Otto
> 

The subtraction a->fr_id - b->fr_id would still done in u_int32_t
even if diff is int64_t as far as I know or you will have to cast
them first.

> > 
> > thanks and
> > regards
> > sashan
> > 
> > --------8<---------------8<---------------8<------------------8<--------
> > diff --git a/sys/net/pf_norm.c b/sys/net/pf_norm.c
> > index 93b98db1d22..f7daa8a0c41 100644
> > --- a/sys/net/pf_norm.c
> > +++ b/sys/net/pf_norm.c
> > @@ -112,7 +112,7 @@ RB_HEAD(pf_frnode_tree, pf_frnode)	pf_frnode_tree;
> >  RB_PROTOTYPE(pf_frnode_tree, pf_frnode, fn_entry, pf_frnode_compare);
> >  RB_GENERATE(pf_frnode_tree, pf_frnode, fn_entry, pf_frnode_compare);
> >  
> > -static __inline int	 pf_frag_compare(struct pf_fragment *,
> > +static __inline int64_t	 pf_frag_compare(struct pf_fragment *,
> >  			    struct pf_fragment *);
> >  RB_PROTOTYPE(pf_frag_tree, pf_fragment, fr_entry, pf_frag_compare);
> >  RB_GENERATE(pf_frag_tree, pf_fragment, fr_entry, pf_frag_compare);
> > @@ -184,10 +184,10 @@ pf_frnode_compare(struct pf_frnode *a, struct pf_frnode *b)
> >  	return (0);
> >  }
> >  
> > -static __inline int
> > +static __inline int64_t
> >  pf_frag_compare(struct pf_fragment *a, struct pf_fragment *b)
> >  {
> > -	int	diff;
> > +	int64_t	diff;
> >  
> >  	if ((diff = a->fr_id - b->fr_id) != 0)
> >  		return (diff);
> >