On Thu, Nov 25, 2021 at 02:56:02PM +0100, Alexandr Nedvedicky wrote:
> Hello,
>
> thank you for taking a look at my diff.
>
> </snip>
>
> > > }
> > >
> > > - if (kif->pfik_ifp != NULL || kif->pfik_group != NULL || kif == pfi_all)
> > > + if (kif->pfik_ifp != NULL || kif->pfik_group != NULL ||kif == pfi_all)
> >
> > Missing space over ^^^ here
>
> fixed, looks like unintended change
> > > + if (found == 0) {
> > > + if (name == NULL)
> > > + return (0);
> > > +
> > > + n = strlen(name);
> > > + if ((n < 1) || (n >= IFNAMSIZ))
> > I would just use:
> > if (n < 1 || n >= IFNAMSIZ)
> > like in other places.
>
> sure.
>
> >
> > > + return (0);
> > > +
> > > + if (((name[0] < 'a') || (name[0] > 'z')) ||
> > > + ((name[0] < 'A') && (name[0] > 'Z')))
> > > + return (0);
> >
> > Not sure what you're after here. The logic of this construct is incorrect.
> > I would steal the defines from libsa/stand.h and then us !isalpha:
> >
> > #define isupper(c) ((c) >= 'A' && (c) <= 'Z')
> > #define islower(c) ((c) >= 'a' && (c) <= 'z')
> > #define isalpha(c) (isupper(c)||islower(c))
> >
> > if (!isalpha(name[0]))
> > return (0);
> >
> > You can also expand the defines if you want.
> > I feel the intention here is to check if this is a interface group. So
> > maybe using the check in pfi_skip_if() would be better:
> >
> > if (name[n-1] >= '0' && name[n-1] <= '9')
> > return (0); /* group names may not end in a digit */
> >
>
> it is just sanity check we deal with either interface or group,
> so isalpha(name[0]) is exactly what I want. And thanks for tip
> to steal it from libsa/stand.h. Perhaps one day some day there
> will be ctype.h for for kernel as well.
>
> the reason to add this extra check is that I want to be sure we eventually
> don't create interface, which starts with space (white char).
>
> Remember former code did not create interface (kif object), it just walked
> through list of existing interfaces in table. Now the DIOCSETIFFLAG ioctl
> needs to be more cautious.
>
>
> updated diff is below.
One more thing to consider, I think the following test in pfi_set_flags():
> + if ((p->pfik_flags_new != p->pfik_flags) &&
> + (p->pfik_flagrefs == 0))
> + pfi_kif_ref(p, PFI_KIF_REF_FLAG);
should actually check for the PFI_IFLAG_SKIP flag and not any flag.
if (ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
p->pfik_flagrefs == 0)
pfi_kif_ref(p, PFI_KIF_REF_FLAG);
Same goes for pfi_clear_flags() just in reverse:
> + if ((p->pfik_flags_new != p->pfik_flags) &&
> + (p->pfik_flagrefs == 1))
> + pfi_kif_unref(p, PFI_KIF_REF_FLAG);
Should be changed to:
if (!ISSET(p->pfik_flags_new, PFI_IFLAG_SKIP) &&
p->pfik_flagrefs == 1)
pfi_kif_unref(p, PFI_KIF_REF_FLAG);
We only want to track the PFI_IFLAG_SKIP flag but not any other flag like
PFI_IFLAG_ANY. At least I think we want to do that, but then I guess
pfi_set_flags() should only add a kif
if (found == 0 && ISSET(flags, PFI_IFLAG_SKIP))
I don't really like pfi_set_flags() and pfi_clear_flags()
and their ioctls DIOCSETIFFLAG and DIOCCLRIFFLAG. There are no checks for
valid flag combinations. So anything goes for these functions.
Also should the name check not happen in the ioctl handler and return
EINVAL for bad input?
> --------8<---------------8<---------------8<------------------8<--------
> index 8de37375ab4..383b8c38f6a 100644
> --- a/sys/net/pf_if.c
> +++ b/sys/net/pf_if.c
> +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);
> +}
> +
> void
> pfi_group_addmember(const char *group, struct ifnet *ifp)
> {
> @@ -361,7 +395,7 @@ pfi_group_addmember(const char *group, struct ifnet *ifp)
> 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 |= gkif->pfik_flags;
> + ikif->pfik_flags_new = ikif->pfik_flags | gkif->pfik_flags;
>
> pfi_group_change(group);
> }
> @@ -786,25 +820,64 @@ int
> pfi_set_flags(const char *name, int flags)
> {
> struct pfi_kif *p;
> + int found = 0;
> + size_t n;
>
> RB_FOREACH(p, pfi_ifhead, &pfi_ifs) {
> if (pfi_skip_if(name, p))
> continue;
> p->pfik_flags_new = p->pfik_flags | flags;
> + found = 1;
> }
> +
> + if (found == 0) {
> + if (name == NULL)
> + return (0);
> +
> + n = strlen(name);
> + if (n < 1 || n >= IFNAMSIZ)
> + return (0);
> +
> + if (!isalpha(name[0]))
> + return (0);
> +
> + p = pfi_kif_get(name, NULL);
> + if (p != NULL) {
> + p->pfik_flags_new = p->pfik_flags | flags;
> +
> + /*
> + * We use pfik_flagrefs counter as an indication
> + * whether the kif has been created on behalf of
> + * 'pfi_set_flags()' or not.
> + */
> + KASSERT((p->pfik_flagrefs == 0) ||
> + (p->pfik_flagrefs == 1));
> +
> + if ((p->pfik_flags_new != p->pfik_flags) &&
> + (p->pfik_flagrefs == 0))
> + pfi_kif_ref(p, PFI_KIF_REF_FLAG);
> + }
> + }
> +
> return (0);
> }
>
> int
> pfi_clear_flags(const char *name, int flags)
> {
> - struct pfi_kif *p;
> + struct pfi_kif *p, *w;
>
> - RB_FOREACH(p, pfi_ifhead, &pfi_ifs) {
> + RB_FOREACH_SAFE(p, pfi_ifhead, &pfi_ifs, w) {
> if (pfi_skip_if(name, p))
> continue;
> +
> + KASSERT((p->pfik_flagrefs == 0) || (p->pfik_flagrefs == 1));
> p->pfik_flags_new = p->pfik_flags & ~flags;
> + if ((p->pfik_flags_new != p->pfik_flags) &&
> + (p->pfik_flagrefs == 1))
> + pfi_kif_unref(p, PFI_KIF_REF_FLAG);
> }
> +
> return (0);
> }
>
--
:wq Claudio
