Re: svn commit: r343213 - in head/sys: net80211 sys

2019-01-20 Thread Andriy Voskoboinyk 
Sun, 20 Jan 2019 16:02:08 +0200 було написано Bjoern A. Zeeb  
:



On 20 Jan 2019, at 13:39, Andriy Voskoboinyk wrote:


Author: avos
Date: Sun Jan 20 13:39:18 2019
New Revision: 343213
URL: https://svnweb.freebsd.org/changeset/base/343213

Log:
  net80211: resolve ioctl <-> detach race for ieee80211com structure

  Since r287197 ieee80211com is a part of drivers softc; as a result,
  after detach all pointers to it (iv_ic, ni_ic) are invalid. Most
  possible users (tasks, interrupt handlers) are blocked / removed
  when device is stopped; however, ioctl handlers were not tracked
  and may crash if ieee80211com structure is accessed.

  Since ieee80211com pointer access from ieee80211vap structure is not
  protected by lock (constant after interface creation) and used in
  many other places just use reference counting for ioctl handlers;
  on detach set 'detached' flag and wait until reference counter goes  
to 0.


So how do any cloned interfaces do this (wifi or non-wifi)?  Is this a  
more general problem or are some wifi drivers just not exactly careful  
with the order they take things down?




That's for wifi only; ifp (and vap as subpart) is alive until
reference counter for ifp is not 0; however, 'com' gets invalid
as soon as device detach procedure is finished - and net80211
uses it in various places inside ieee80211_ioctl().

On another note, why would refcount(9) not be sufficient?  I didn’t  
really like the MC() macros and the hand crafted state machine for a  
refcount when scrolling through.




Just to keep 'detached' flag and reference counter inside one variable
(they both need to be atomically accessible).


/bz

___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r343213 - in head/sys: net80211 sys

2019-01-20 Thread Bjoern A. Zeeb 

On 20 Jan 2019, at 13:39, Andriy Voskoboinyk wrote:


Author: avos
Date: Sun Jan 20 13:39:18 2019
New Revision: 343213
URL: https://svnweb.freebsd.org/changeset/base/343213

Log:
  net80211: resolve ioctl <-> detach race for ieee80211com structure

  Since r287197 ieee80211com is a part of drivers softc; as a result,
  after detach all pointers to it (iv_ic, ni_ic) are invalid. Most
  possible users (tasks, interrupt handlers) are blocked / removed
  when device is stopped; however, ioctl handlers were not tracked
  and may crash if ieee80211com structure is accessed.

  Since ieee80211com pointer access from ieee80211vap structure is not
  protected by lock (constant after interface creation) and used in
  many other places just use reference counting for ioctl handlers;
  on detach set 'detached' flag and wait until reference counter goes 
to 0.


So how do any cloned interfaces do this (wifi or non-wifi)?  Is this a 
more general problem or are some wifi drivers just not exactly careful 
with the order they take things down?


On another note, why would refcount(9) not be sufficient?  I didn’t 
really like the MC() macros and the hand crafted state machine for a 
refcount when scrolling through.


/bz
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

svn commit: r343213 - in head/sys: net80211 sys

2019-01-20 Thread Andriy Voskoboinyk 
Author: avos
Date: Sun Jan 20 13:39:18 2019
New Revision: 343213
URL: https://svnweb.freebsd.org/changeset/base/343213

Log:
  net80211: resolve ioctl <-> detach race for ieee80211com structure
  
  Since r287197 ieee80211com is a part of drivers softc; as a result,
  after detach all pointers to it (iv_ic, ni_ic) are invalid. Most
  possible users (tasks, interrupt handlers) are blocked / removed
  when device is stopped; however, ioctl handlers were not tracked
  and may crash if ieee80211com structure is accessed.
  
  Since ieee80211com pointer access from ieee80211vap structure is not
  protected by lock (constant after interface creation) and used in
  many other places just use reference counting for ioctl handlers;
  on detach set 'detached' flag and wait until reference counter goes to 0.
  
  For HEAD ieee80211vap size was changed (__FreeBSD_version bumped);
  however, in stable branches I'm going to split / reuse the last
  iv_spare field for KBI stability.
  
  Tested with:
   - rsu(4), SIOCSIFCAP (-rxcsum) ioctl;
   - rtwn_pci(4), SIOCG80211 / IEEE80211_IOC_HTPROTMODE ioctl.
  
  MFC after:1 week

Modified:
  head/sys/net80211/ieee80211.c
  head/sys/net80211/ieee80211_freebsd.c
  head/sys/net80211/ieee80211_freebsd.h
  head/sys/net80211/ieee80211_ioctl.c
  head/sys/net80211/ieee80211_var.h
  head/sys/sys/param.h

Modified: head/sys/net80211/ieee80211.c
==
--- head/sys/net80211/ieee80211.c   Sun Jan 20 13:16:36 2019
(r343212)
+++ head/sys/net80211/ieee80211.c   Sun Jan 20 13:39:18 2019
(r343213)
@@ -405,8 +405,10 @@ ieee80211_ifdetach(struct ieee80211com *ic)
 * The VAP is responsible for setting and clearing
 * the VIMAGE context.
 */
-   while ((vap = TAILQ_FIRST(&ic->ic_vaps)) != NULL)
+   while ((vap = TAILQ_FIRST(&ic->ic_vaps)) != NULL) {
+   ieee80211_com_vdetach(vap);
ieee80211_vap_destroy(vap);
+   }
ieee80211_waitfor_parent(ic);
 
ieee80211_sysctl_detach(ic);

Modified: head/sys/net80211/ieee80211_freebsd.c
==
--- head/sys/net80211/ieee80211_freebsd.c   Sun Jan 20 13:16:36 2019
(r343212)
+++ head/sys/net80211/ieee80211_freebsd.c   Sun Jan 20 13:39:18 2019
(r343213)
@@ -307,6 +307,55 @@ ieee80211_sysctl_vdetach(struct ieee80211vap *vap)
}
 }
 
+#defineMS(_v, _f)  (((_v) & _f##_M) >> _f##_S)
+int
+ieee80211_com_vincref(struct ieee80211vap *vap)
+{
+   uint32_t ostate;
+
+   ostate = atomic_fetchadd_32(&vap->iv_com_state, IEEE80211_COM_REF_ADD);
+
+   if (ostate & IEEE80211_COM_DETACHED) {
+   atomic_subtract_32(&vap->iv_com_state, IEEE80211_COM_REF_ADD);
+   return (ENETDOWN);
+   }
+
+   if (MS(ostate, IEEE80211_COM_REF) == IEEE80211_COM_REF_MAX) {
+   atomic_subtract_32(&vap->iv_com_state, IEEE80211_COM_REF_ADD);
+   return (EOVERFLOW);
+   }
+
+   return (0);
+}
+
+void
+ieee80211_com_vdecref(struct ieee80211vap *vap)
+{
+   uint32_t ostate;
+
+   ostate = atomic_fetchadd_32(&vap->iv_com_state, -IEEE80211_COM_REF_ADD);
+
+   KASSERT(MS(ostate, IEEE80211_COM_REF) != 0,
+   ("com reference counter underflow"));
+
+   (void) ostate;
+}
+
+void
+ieee80211_com_vdetach(struct ieee80211vap *vap)
+{
+   int sleep_time;
+
+   sleep_time = msecs_to_ticks(250);
+   if (sleep_time == 0)
+   sleep_time = 1;
+
+   atomic_set_32(&vap->iv_com_state, IEEE80211_COM_DETACHED);
+   while (MS(atomic_load_32(&vap->iv_com_state), IEEE80211_COM_REF) != 0)
+   pause("comref", sleep_time);
+}
+#undef MS
+
 int
 ieee80211_node_dectestref(struct ieee80211_node *ni)
 {

Modified: head/sys/net80211/ieee80211_freebsd.h
==
--- head/sys/net80211/ieee80211_freebsd.h   Sun Jan 20 13:16:36 2019
(r343212)
+++ head/sys/net80211/ieee80211_freebsd.h   Sun Jan 20 13:39:18 2019
(r343213)
@@ -224,6 +224,11 @@ typedef struct mtx ieee80211_rt_lock_t;
  */
 #include 
 
+struct ieee80211vap;
+intieee80211_com_vincref(struct ieee80211vap *);
+void   ieee80211_com_vdecref(struct ieee80211vap *);
+void   ieee80211_com_vdetach(struct ieee80211vap *);
+
 #define ieee80211_node_initref(_ni) \
do { ((_ni)->ni_refcnt = 1); } while (0)
 #define ieee80211_node_incref(_ni) \
@@ -235,7 +240,6 @@ int ieee80211_node_dectestref(struct ieee80211_node *n
 #defineieee80211_node_refcnt(_ni)  (_ni)->ni_refcnt
 
 struct ifqueue;
-struct ieee80211vap;
 void   ieee80211_drain_ifq(struct ifqueue *);
 void   ieee80211_flush_ifq(struct ifqueue *, struct ieee80211vap *);
 

Modified: head/sys/net80211/ieee80211_ioctl.c
==