On Sat, Dec 04, 2021 at 07:01:23PM +0100, Alexandr Nedvedicky wrote:
> Hello,
>
> </snip>
> On Fri, Dec 03, 2021 at 03:42:09PM +0100, Claudio Jeker wrote:
> >
> > See comments below.
> >
> >
> > > +void
> > > +pfi_group_delmember(const char *group, struct ifnet *ifp)
> > > +{
> > > + struct pfi_kif *gkif, *ikif;
> > > +
> > > + if ((gkif = pfi_kif_get(group, NULL)) == NULL ||
> > > + (ikif = pfi_kif_get(ifp->if_xname, NULL)) == NULL)
> > > + panic("%s: pfi_kif_get failed", __func__);
> > > + ikif->pfik_flags_new = ikif->pfik_flags & ~gkif->pfik_flags;
> > > +
> > > + pfi_group_change(group);
> > > +}
> > > +
> >
> > This function is not quite right. For example:
> >
> > ifconfig vio0 group foo group bar
> > pfctl -f - <<EOF
> > set skip on { foo bar }
> > block return
> > EOF
> > ping -qc2 100.64.1.2
> >
> > PING 100.64.1.2 (100.64.1.2): 56 data bytes
> >
> > --- 100.64.1.2 ping statistics ---
> > 2 packets transmitted, 2 packets received, 0.0% packet loss
> > round-trip min/avg/max/std-dev = 0.153/0.178/0.202/0.024 ms
> >
> >
> > Now lets remove just group bar from the interface.
> >
> > ifconfig vio0 -group bar
> > ping -qc2 100.64.1.2
> >
> > PING 100.64.1.2 (100.64.1.2): 56 data bytes
> > ping: sendmsg: Permission denied
> > ping: wrote 100.64.1.2 64 chars, ret=-1
> > ping: sendmsg: Permission denied
> > ping: wrote 100.64.1.2 64 chars, ret=-1
> >
> > pfi_group_delmember() does not take into consideration other groups
> > (including the interface itself) that may still allow PFI_IFLAG_SKIP.
> > It just clears the flag.
> >
> > I think on delete the flag needs to be recalculated after the group has
> > been removed. Not sure if this is easy possible though.
> >
>
> yes, you are absolutely right. the current logic around pfi_skip_if() is
> bit convoluted. in order to fix it I must rework pfi_xcommit() and
> related
> set_flags()/clear_flags() functions. The idea is to let pfi_xcommit() to
> combine skip flags from all groups, which are attached to interface.
>
>
> updated diff is attached.
One comment below but this diff is OK claudio@
> thanks and
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> void
> pfi_xcommit(void)
> {
> - struct pfi_kif *p;
> + struct pfi_kif *p, *gkif;
> + struct ifg_list *g;
> + struct ifnet *ifp;
> + size_t n;
>
> - RB_FOREACH(p, pfi_ifhead, &pfi_ifs)
> + RB_FOREACH(p, pfi_ifhead, &pfi_ifs) {
> p->pfik_flags = p->pfik_flags_new;
> + n = strlen(p->pfik_name);
> + ifp = p->pfik_ifp;
> + /*
> + * if kif is backed by existing interface, then we must use
> + * skip flags found in groups. We use pfik_flags_new, otherwise
> + * we would need to do two RB_FOREACH() passes: the first to
> + * commit group changes the second to commit flag changes for
> + * interfaces.
> + */
> + if (isdigit(p->pfik_name[n - 1]) && ifp != NULL)
No other code in pf_if.c is checking both pfik_name and ifp != NULL in
similar situations. I think you should skip the isdigit() check here.
If there is a real interface connected to a pfi_kif than it should be updated.
Lets not introduce more complexity here.
> + TAILQ_FOREACH(g, &ifp->if_groups, ifgl_next) {
> + gkif =
> + (struct pfi_kif *)g->ifgl_group->ifg_pf_kif;
> + KASSERT(gkif != NULL);
> + p->pfik_flags |= gkif->pfik_flags_new;
> + }
> + }
> }
--
:wq Claudio
