> On 5 Oct 2023, at 00:31, Alexander Bluhm <alexander.bl...@gmx.net> wrote:
> 
> On Wed, Oct 04, 2023 at 11:03:27PM +0300, Vitaliy Makkoveev wrote:
>> On Wed, Oct 04, 2023 at 09:13:59PM +0200, Alexander Bluhm wrote:
>>> On Wed, Oct 04, 2023 at 08:42:48PM +0200, Kirill Miazine wrote:
>>>>> If it happns again, could you send an 'ps axlww | grep ifconifg'
>>>>> output?  Then we see the wait channel where it hangs in the kernel.
>>>>> 
>>>>> $ ps axlww
>>>>>   UID   PID  PPID CPU PRI  NI   VSZ   RSS WCHAN   STAT   TT       TIME 
>>>>> COMMAND
>>>> 
>>>> Here it happened again:
>>>> 
>>>>     0 75339 23922   0  10   0   360   296 wg_ifq  D+U    p0    0:00.00 
>>>> ifconfig wg1 destroy
>>> 
>>> wg_peer_destroy()
>>>     ...
>>>        NET_LOCK();
>>>        while (!ifq_empty(&sc->sc_if.if_snd)) {
>>>                NET_UNLOCK();
>>>                tsleep_nsec(sc, PWAIT, "wg_ifq", 1000);
>>>                NET_LOCK();
>>>        }
>>>        NET_UNLOCK();
>>> 
>>> This net lock dance looks fishy.  And the sleep has a timeout of 1
>>> milli second.  But that is may be per packet.  So if you have a
>>> long queue or the queue refills somehow, it will take forever.
>>> 
>>> I think the difference in the usage is constant traffic that keeps
>>> the send queue full.  The timeout hides the problem when there are
>>> only a few packets.
>>> 
>> 
>> This should ensure wg_qstart() will not dereference dying `peer'. Looks
>> crappy and potentially could block forever, but should work. However
>> netlock it unnecessary here. netlocked wg_output() could fill `if_snd'
>> while netlock released before tsleep(), so it serializes nothing but
>> stops packets processing.
>> 
>> Kirill, does this diff help? 
> 
> I doubt that it changes much.  When netlock is not taken, the queue
> can still be filled with packets.
> 
> Removing this ugly netlock makes sense anyway.  But without any
> synchronisation just reading a variable feels wrong.  Can we add a
> read once like for mq_len in sys/mbuf.h?  And the ifq_set_maxlen()
> also looks very unsafe.  For mbuf queues I added a mutex, interface
> queues should do the same.
> 
> ok?
> 

I guess this is uniprocessor machine, so synchronisation is not
related.

diff is ok mvs.

> bluhm
> 
> Index: net/ifq.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/ifq.c,v
> retrieving revision 1.50
> diff -u -p -r1.50 ifq.c
> --- net/ifq.c 30 Jul 2023 05:39:52 -0000      1.50
> +++ net/ifq.c 4 Oct 2023 21:04:20 -0000
> @@ -529,6 +529,14 @@ ifq_hdatalen(struct ifqueue *ifq)
>       return (len);
> }
> 
> +void
> +ifq_set_maxlen(struct ifqueue *ifq, unsigned int maxlen)
> +{
> +     mtx_enter(&ifq->ifq_mtx);
> +     ifq->ifq_maxlen = maxlen;
> +     mtx_leave(&ifq->ifq_mtx);
> +}
> +
> unsigned int
> ifq_purge(struct ifqueue *ifq)
> {
> Index: net/ifq.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/ifq.h,v
> retrieving revision 1.38
> diff -u -p -r1.38 ifq.h
> --- net/ifq.h 30 Jul 2023 05:39:52 -0000      1.38
> +++ net/ifq.h 4 Oct 2023 21:09:04 -0000
> @@ -435,6 +435,7 @@ void               ifq_deq_commit(struct ifqueue *, 
> void           ifq_deq_rollback(struct ifqueue *, struct mbuf *);
> struct mbuf   *ifq_dequeue(struct ifqueue *);
> int            ifq_hdatalen(struct ifqueue *);
> +void          ifq_set_maxlen(struct ifqueue *, unsigned int);
> void           ifq_mfreem(struct ifqueue *, struct mbuf *);
> void           ifq_mfreeml(struct ifqueue *, struct mbuf_list *);
> unsigned int   ifq_purge(struct ifqueue *);
> @@ -448,9 +449,8 @@ int                ifq_deq_sleep(struct ifqueue *, st
>                    const char *, volatile unsigned int *,
>                    volatile unsigned int *);
> 
> -#define      ifq_len(_ifq)                   ((_ifq)->ifq_len)
> -#define      ifq_empty(_ifq)                 (ifq_len(_ifq) == 0)
> -#define      ifq_set_maxlen(_ifq, _l)        ((_ifq)->ifq_maxlen = (_l))
> +#define ifq_len(_ifq)                READ_ONCE((_ifq)->ifq_len)
> +#define ifq_empty(_ifq)              (ifq_len(_ifq) == 0)
> 
> static inline int
> ifq_is_priq(struct ifqueue *ifq)
> @@ -490,8 +490,8 @@ int                ifiq_input(struct ifiqueue *, stru
> int            ifiq_enqueue(struct ifiqueue *, struct mbuf *);
> void           ifiq_add_data(struct ifiqueue *, struct if_data *);
> 
> -#define      ifiq_len(_ifiq)                 ml_len(&(_ifiq)->ifiq_ml)
> -#define      ifiq_empty(_ifiq)               ml_empty(&(_ifiq)->ifiq_ml)
> +#define ifiq_len(_ifiq)              READ_ONCE(ml_len(&(_ifiq)->ifiq_ml))
> +#define ifiq_empty(_ifiq)    (ifiq_len(_ifiq) == 0)
> 
> #endif /* _KERNEL */

Reply via email to