Re: ifg_refcnt atomic operation
On Sat, Feb 06, 2021 at 04:44:08PM +0100, Alexander Bluhm wrote:
> Or should we go with a self crafted ++ -- refcounting?
This would look like this, also fine with me. kasserts are also in
refcnt_... API.
ok?
bluhm
Index: net/if.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
retrieving revision 1.626
diff -u -p -r1.626 if.c
--- net/if.c1 Feb 2021 07:43:33 - 1.626
+++ net/if.c6 Feb 2021 16:26:51 -
@@ -2601,7 +2601,7 @@ if_creategroup(const char *groupname)
return (NULL);
strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group));
- ifg->ifg_refcnt = 0;
+ ifg->ifg_refcnt = 1;
ifg->ifg_carp_demoted = 0;
TAILQ_INIT(&ifg->ifg_members);
#if NPF > 0
@@ -2642,13 +2642,18 @@ if_addgroup(struct ifnet *ifp, const cha
if (!strcmp(ifg->ifg_group, groupname))
break;
- if (ifg == NULL && (ifg = if_creategroup(groupname)) == NULL) {
+ if (ifg == NULL)
+ ifg = if_creategroup(groupname);
+ else
+ ifg->ifg_refcnt++;
+ KASSERT(ifg->ifg_refcnt != 0);
+
+ if (ifg == NULL) {
free(ifgl, M_TEMP, sizeof(*ifgl));
free(ifgm, M_TEMP, sizeof(*ifgm));
return (ENOMEM);
}
- ifg->ifg_refcnt++;
ifgl->ifgl_group = ifg;
ifgm->ifgm_ifp = ifp;
@@ -2692,6 +2697,7 @@ if_delgroup(struct ifnet *ifp, const cha
pfi_group_change(groupname);
#endif
+ KASSERT(ifgl->ifgl_group->ifg_refcnt != 0);
if (--ifgl->ifgl_group->ifg_refcnt == 0) {
TAILQ_REMOVE(&ifg_head, ifgl->ifgl_group, ifg_next);
#if NPF > 0
Index: netinet/ip_carp.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.351
diff -u -p -r1.351 ip_carp.c
--- netinet/ip_carp.c 21 Jan 2021 13:18:07 - 1.351
+++ netinet/ip_carp.c 6 Feb 2021 14:45:34 -
@@ -786,10 +786,7 @@ carp_sysctl(int *name, u_int namelen, vo
void
carpattach(int n)
{
- struct ifg_group*ifg;
-
- if ((ifg = if_creategroup("carp")) != NULL)
- ifg->ifg_refcnt++; /* keep around even if empty */
+ if_creategroup("carp"); /* keep around even if empty */
if_clone_attach(&carp_cloner);
carpcounters = counters_alloc(carps_ncounters);
}
Re: ifg_refcnt atomic operation
On Sat, Feb 06, 2021 at 05:58:35PM +0300, Vitaliy Makkoveev wrote:
> I???m not sure it should be atomic. It seems groups require their own
> lock and this lock should be held while we perform if_addgroup() and
> if_delgroup().
I also think that atomic refcounting is not needed here. But it
does no harm as adding interface groups is not performance critical.
Question is if we want to use refcnt_... API even if it does more
than required. Or should we go with a self crafted ++ -- refcounting?
I think the API provides a nicer interface.
bluhm
> However if_creategroup() should set `ifg_refcnt??? to 1
> and carp(4) should not touch groups internals.
>
> >
> > Index: net/if.c
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
> > retrieving revision 1.626
> > diff -u -p -r1.626 if.c
> > --- net/if.c1 Feb 2021 07:43:33 - 1.626
> > +++ net/if.c6 Feb 2021 12:16:20 -
> > @@ -2601,7 +2601,7 @@ if_creategroup(const char *groupname)
> > return (NULL);
> >
> > strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group));
> > - ifg->ifg_refcnt = 0;
> > + refcnt_init(&ifg->ifg_refcnt);
> > ifg->ifg_carp_demoted = 0;
> > TAILQ_INIT(&ifg->ifg_members);
> > #if NPF > 0
> > @@ -2642,13 +2642,17 @@ if_addgroup(struct ifnet *ifp, const cha
> > if (!strcmp(ifg->ifg_group, groupname))
> > break;
> >
> > - if (ifg == NULL && (ifg = if_creategroup(groupname)) == NULL) {
> > + if (ifg == NULL)
> > + ifg = if_creategroup(groupname);
> > + else
> > + refcnt_take(&ifg->ifg_refcnt);
> > +
> > + if (ifg == NULL) {
> > free(ifgl, M_TEMP, sizeof(*ifgl));
> > free(ifgm, M_TEMP, sizeof(*ifgm));
> > return (ENOMEM);
> > }
> >
> > - ifg->ifg_refcnt++;
> > ifgl->ifgl_group = ifg;
> > ifgm->ifgm_ifp = ifp;
> >
> > @@ -2692,7 +2696,7 @@ if_delgroup(struct ifnet *ifp, const cha
> > pfi_group_change(groupname);
> > #endif
> >
> > - if (--ifgl->ifgl_group->ifg_refcnt == 0) {
> > + if (refcnt_rele(&ifgl->ifgl_group->ifg_refcnt)) {
> > TAILQ_REMOVE(&ifg_head, ifgl->ifgl_group, ifg_next);
> > #if NPF > 0
> > pfi_detach_ifgroup(ifgl->ifgl_group);
> > Index: net/if_var.h
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_var.h,v
> > retrieving revision 1.112
> > diff -u -p -r1.112 if_var.h
> > --- net/if_var.h29 Jul 2020 12:09:31 - 1.112
> > +++ net/if_var.h6 Feb 2021 12:11:35 -
> > @@ -263,7 +263,7 @@ struct ifmaddr {
> >
> > struct ifg_group {
> > char ifg_group[IFNAMSIZ];
> > - u_intifg_refcnt;
> > + struct refcntifg_refcnt;
> > caddr_t ifg_pf_kif;
> > int ifg_carp_demoted;
> > TAILQ_HEAD(, ifg_member) ifg_members;
> > Index: netinet/ip_carp.c
> > ===
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_carp.c,v
> > retrieving revision 1.351
> > diff -u -p -r1.351 ip_carp.c
> > --- netinet/ip_carp.c 21 Jan 2021 13:18:07 - 1.351
> > +++ netinet/ip_carp.c 6 Feb 2021 12:11:35 -
> > @@ -786,10 +786,7 @@ carp_sysctl(int *name, u_int namelen, vo
> > void
> > carpattach(int n)
> > {
> > - struct ifg_group*ifg;
> > -
> > - if ((ifg = if_creategroup("carp")) != NULL)
> > - ifg->ifg_refcnt++; /* keep around even if empty */
> > + if_creategroup("carp"); /* keep around even if empty */
> > if_clone_attach(&carp_cloner);
> > carpcounters = counters_alloc(carps_ncounters);
> > }
> >
>
Re: ifg_refcnt atomic operation
> On 6 Feb 2021, at 15:23, Alexander Bluhm wrote:
>
> On Sat, Feb 06, 2021 at 05:04:20PM +1000, David Gwynne wrote:
>> refcnt_init starts counting at 1, while the existing code starts at 0. Do
>> the crashes stop because we never fully release all the references and
>> never free it now?
>
> You are absolutely right. I was too optimistic.
>
> Correct diff is below. It does not fix anything. Only advantage
> is that carp does not access interface group internals.
>
> bluhm
I’m not sure it should be atomic. It seems groups require their own
lock and this lock should be held while we perform if_addgroup() and
if_delgroup(). However if_creategroup() should set `ifg_refcnt’ to 1
and carp(4) should not touch groups internals.
>
> Index: net/if.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
> retrieving revision 1.626
> diff -u -p -r1.626 if.c
> --- net/if.c 1 Feb 2021 07:43:33 - 1.626
> +++ net/if.c 6 Feb 2021 12:16:20 -
> @@ -2601,7 +2601,7 @@ if_creategroup(const char *groupname)
> return (NULL);
>
> strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group));
> - ifg->ifg_refcnt = 0;
> + refcnt_init(&ifg->ifg_refcnt);
> ifg->ifg_carp_demoted = 0;
> TAILQ_INIT(&ifg->ifg_members);
> #if NPF > 0
> @@ -2642,13 +2642,17 @@ if_addgroup(struct ifnet *ifp, const cha
> if (!strcmp(ifg->ifg_group, groupname))
> break;
>
> - if (ifg == NULL && (ifg = if_creategroup(groupname)) == NULL) {
> + if (ifg == NULL)
> + ifg = if_creategroup(groupname);
> + else
> + refcnt_take(&ifg->ifg_refcnt);
> +
> + if (ifg == NULL) {
> free(ifgl, M_TEMP, sizeof(*ifgl));
> free(ifgm, M_TEMP, sizeof(*ifgm));
> return (ENOMEM);
> }
>
> - ifg->ifg_refcnt++;
> ifgl->ifgl_group = ifg;
> ifgm->ifgm_ifp = ifp;
>
> @@ -2692,7 +2696,7 @@ if_delgroup(struct ifnet *ifp, const cha
> pfi_group_change(groupname);
> #endif
>
> - if (--ifgl->ifgl_group->ifg_refcnt == 0) {
> + if (refcnt_rele(&ifgl->ifgl_group->ifg_refcnt)) {
> TAILQ_REMOVE(&ifg_head, ifgl->ifgl_group, ifg_next);
> #if NPF > 0
> pfi_detach_ifgroup(ifgl->ifgl_group);
> Index: net/if_var.h
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_var.h,v
> retrieving revision 1.112
> diff -u -p -r1.112 if_var.h
> --- net/if_var.h 29 Jul 2020 12:09:31 - 1.112
> +++ net/if_var.h 6 Feb 2021 12:11:35 -
> @@ -263,7 +263,7 @@ struct ifmaddr {
>
> struct ifg_group {
> char ifg_group[IFNAMSIZ];
> - u_intifg_refcnt;
> + struct refcntifg_refcnt;
> caddr_t ifg_pf_kif;
> int ifg_carp_demoted;
> TAILQ_HEAD(, ifg_member) ifg_members;
> Index: netinet/ip_carp.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.351
> diff -u -p -r1.351 ip_carp.c
> --- netinet/ip_carp.c 21 Jan 2021 13:18:07 - 1.351
> +++ netinet/ip_carp.c 6 Feb 2021 12:11:35 -
> @@ -786,10 +786,7 @@ carp_sysctl(int *name, u_int namelen, vo
> void
> carpattach(int n)
> {
> - struct ifg_group*ifg;
> -
> - if ((ifg = if_creategroup("carp")) != NULL)
> - ifg->ifg_refcnt++; /* keep around even if empty */
> + if_creategroup("carp"); /* keep around even if empty */
> if_clone_attach(&carp_cloner);
> carpcounters = counters_alloc(carps_ncounters);
> }
>
Re: ifg_refcnt atomic operation
On Sat, Feb 06, 2021 at 05:04:20PM +1000, David Gwynne wrote:
> refcnt_init starts counting at 1, while the existing code starts at 0. Do
> the crashes stop because we never fully release all the references and
> never free it now?
You are absolutely right. I was too optimistic.
Correct diff is below. It does not fix anything. Only advantage
is that carp does not access interface group internals.
bluhm
Index: net/if.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
retrieving revision 1.626
diff -u -p -r1.626 if.c
--- net/if.c1 Feb 2021 07:43:33 - 1.626
+++ net/if.c6 Feb 2021 12:16:20 -
@@ -2601,7 +2601,7 @@ if_creategroup(const char *groupname)
return (NULL);
strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group));
- ifg->ifg_refcnt = 0;
+ refcnt_init(&ifg->ifg_refcnt);
ifg->ifg_carp_demoted = 0;
TAILQ_INIT(&ifg->ifg_members);
#if NPF > 0
@@ -2642,13 +2642,17 @@ if_addgroup(struct ifnet *ifp, const cha
if (!strcmp(ifg->ifg_group, groupname))
break;
- if (ifg == NULL && (ifg = if_creategroup(groupname)) == NULL) {
+ if (ifg == NULL)
+ ifg = if_creategroup(groupname);
+ else
+ refcnt_take(&ifg->ifg_refcnt);
+
+ if (ifg == NULL) {
free(ifgl, M_TEMP, sizeof(*ifgl));
free(ifgm, M_TEMP, sizeof(*ifgm));
return (ENOMEM);
}
- ifg->ifg_refcnt++;
ifgl->ifgl_group = ifg;
ifgm->ifgm_ifp = ifp;
@@ -2692,7 +2696,7 @@ if_delgroup(struct ifnet *ifp, const cha
pfi_group_change(groupname);
#endif
- if (--ifgl->ifgl_group->ifg_refcnt == 0) {
+ if (refcnt_rele(&ifgl->ifgl_group->ifg_refcnt)) {
TAILQ_REMOVE(&ifg_head, ifgl->ifgl_group, ifg_next);
#if NPF > 0
pfi_detach_ifgroup(ifgl->ifgl_group);
Index: net/if_var.h
===
RCS file: /data/mirror/openbsd/cvs/src/sys/net/if_var.h,v
retrieving revision 1.112
diff -u -p -r1.112 if_var.h
--- net/if_var.h29 Jul 2020 12:09:31 - 1.112
+++ net/if_var.h6 Feb 2021 12:11:35 -
@@ -263,7 +263,7 @@ struct ifmaddr {
struct ifg_group {
char ifg_group[IFNAMSIZ];
- u_intifg_refcnt;
+ struct refcntifg_refcnt;
caddr_t ifg_pf_kif;
int ifg_carp_demoted;
TAILQ_HEAD(, ifg_member) ifg_members;
Index: netinet/ip_carp.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.351
diff -u -p -r1.351 ip_carp.c
--- netinet/ip_carp.c 21 Jan 2021 13:18:07 - 1.351
+++ netinet/ip_carp.c 6 Feb 2021 12:11:35 -
@@ -786,10 +786,7 @@ carp_sysctl(int *name, u_int namelen, vo
void
carpattach(int n)
{
- struct ifg_group*ifg;
-
- if ((ifg = if_creategroup("carp")) != NULL)
- ifg->ifg_refcnt++; /* keep around even if empty */
+ if_creategroup("carp"); /* keep around even if empty */
if_clone_attach(&carp_cloner);
carpcounters = counters_alloc(carps_ncounters);
}
Re: ifg_refcnt atomic operation
refcnt_init starts counting at 1, while the existing code starts at 0. Do
the crashes stop because we never fully release all the references and
never free it now?
On Sat, 6 Feb 2021, 10:55 Alexander Bluhm, wrote:
> Hi,
>
> When I replace the ++ and -- of ifg_refcnt with an atomic operation,
> it fixes this syzkaller panic.
>
>
> https://syzkaller.appspot.com/bug?id=54e16dc5bce6929e14b42e2f1379f1c18f62be43
>
> Without the fix "syz-execprog -repeat=0 -procs=8 repro-pfi.syz"
> crashes my vmm in a few seconds. With the diff I cannot reproduce
> for several minutes.
>
> ok?
>
> bluhm
>
> Index: net/if.c
> ===
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.626
> diff -u -p -r1.626 if.c
> --- net/if.c1 Feb 2021 07:43:33 - 1.626
> +++ net/if.c6 Feb 2021 00:37:50 -
> @@ -2601,7 +2601,7 @@ if_creategroup(const char *groupname)
> return (NULL);
>
> strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group));
> - ifg->ifg_refcnt = 0;
> + refcnt_init(&ifg->ifg_refcnt);
> ifg->ifg_carp_demoted = 0;
> TAILQ_INIT(&ifg->ifg_members);
> #if NPF > 0
> @@ -2648,7 +2648,7 @@ if_addgroup(struct ifnet *ifp, const cha
> return (ENOMEM);
> }
>
> - ifg->ifg_refcnt++;
> + refcnt_take(&ifg->ifg_refcnt);
> ifgl->ifgl_group = ifg;
> ifgm->ifgm_ifp = ifp;
>
> @@ -2692,7 +2692,7 @@ if_delgroup(struct ifnet *ifp, const cha
> pfi_group_change(groupname);
> #endif
>
> - if (--ifgl->ifgl_group->ifg_refcnt == 0) {
> + if (refcnt_rele(&ifgl->ifgl_group->ifg_refcnt)) {
> TAILQ_REMOVE(&ifg_head, ifgl->ifgl_group, ifg_next);
> #if NPF > 0
> pfi_detach_ifgroup(ifgl->ifgl_group);
> Index: net/if_var.h
> ===
> RCS file: /cvs/src/sys/net/if_var.h,v
> retrieving revision 1.112
> diff -u -p -r1.112 if_var.h
> --- net/if_var.h29 Jul 2020 12:09:31 - 1.112
> +++ net/if_var.h6 Feb 2021 00:38:23 -
> @@ -263,7 +263,7 @@ struct ifmaddr {
>
> struct ifg_group {
> char ifg_group[IFNAMSIZ];
> - u_intifg_refcnt;
> + struct refcntifg_refcnt;
> caddr_t ifg_pf_kif;
> int ifg_carp_demoted;
> TAILQ_HEAD(, ifg_member) ifg_members;
> Index: netinet/ip_carp.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_carp.c,v
> retrieving revision 1.351
> diff -u -p -r1.351 ip_carp.c
> --- netinet/ip_carp.c 21 Jan 2021 13:18:07 - 1.351
> +++ netinet/ip_carp.c 6 Feb 2021 00:39:14 -
> @@ -789,7 +789,7 @@ carpattach(int n)
> struct ifg_group*ifg;
>
> if ((ifg = if_creategroup("carp")) != NULL)
> - ifg->ifg_refcnt++; /* keep around even if empty */
> + refcnt_take(&ifg->ifg_refcnt); /* keep around even if
> empty */
> if_clone_attach(&carp_cloner);
> carpcounters = counters_alloc(carps_ncounters);
> }
>
>
ifg_refcnt atomic operation
Hi,
When I replace the ++ and -- of ifg_refcnt with an atomic operation,
it fixes this syzkaller panic.
https://syzkaller.appspot.com/bug?id=54e16dc5bce6929e14b42e2f1379f1c18f62be43
Without the fix "syz-execprog -repeat=0 -procs=8 repro-pfi.syz"
crashes my vmm in a few seconds. With the diff I cannot reproduce
for several minutes.
ok?
bluhm
Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.626
diff -u -p -r1.626 if.c
--- net/if.c1 Feb 2021 07:43:33 - 1.626
+++ net/if.c6 Feb 2021 00:37:50 -
@@ -2601,7 +2601,7 @@ if_creategroup(const char *groupname)
return (NULL);
strlcpy(ifg->ifg_group, groupname, sizeof(ifg->ifg_group));
- ifg->ifg_refcnt = 0;
+ refcnt_init(&ifg->ifg_refcnt);
ifg->ifg_carp_demoted = 0;
TAILQ_INIT(&ifg->ifg_members);
#if NPF > 0
@@ -2648,7 +2648,7 @@ if_addgroup(struct ifnet *ifp, const cha
return (ENOMEM);
}
- ifg->ifg_refcnt++;
+ refcnt_take(&ifg->ifg_refcnt);
ifgl->ifgl_group = ifg;
ifgm->ifgm_ifp = ifp;
@@ -2692,7 +2692,7 @@ if_delgroup(struct ifnet *ifp, const cha
pfi_group_change(groupname);
#endif
- if (--ifgl->ifgl_group->ifg_refcnt == 0) {
+ if (refcnt_rele(&ifgl->ifgl_group->ifg_refcnt)) {
TAILQ_REMOVE(&ifg_head, ifgl->ifgl_group, ifg_next);
#if NPF > 0
pfi_detach_ifgroup(ifgl->ifgl_group);
Index: net/if_var.h
===
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.112
diff -u -p -r1.112 if_var.h
--- net/if_var.h29 Jul 2020 12:09:31 - 1.112
+++ net/if_var.h6 Feb 2021 00:38:23 -
@@ -263,7 +263,7 @@ struct ifmaddr {
struct ifg_group {
char ifg_group[IFNAMSIZ];
- u_intifg_refcnt;
+ struct refcntifg_refcnt;
caddr_t ifg_pf_kif;
int ifg_carp_demoted;
TAILQ_HEAD(, ifg_member) ifg_members;
Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.351
diff -u -p -r1.351 ip_carp.c
--- netinet/ip_carp.c 21 Jan 2021 13:18:07 - 1.351
+++ netinet/ip_carp.c 6 Feb 2021 00:39:14 -
@@ -789,7 +789,7 @@ carpattach(int n)
struct ifg_group*ifg;
if ((ifg = if_creategroup("carp")) != NULL)
- ifg->ifg_refcnt++; /* keep around even if empty */
+ refcnt_take(&ifg->ifg_refcnt); /* keep around even if empty */
if_clone_attach(&carp_cloner);
carpcounters = counters_alloc(carps_ncounters);
}
