Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <09834e8d-ca48-e21d-fd96-9de87294a7f4@ispras.ru>
Date:   Fri, 26 May 2023 20:35:25 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] batman-adv: Broken sync while rescheduling delayed work
Content-Language: ru
To:     Sven Eckelmann <sven@narfation.org>,
        Marek Lindner <mareklindner@neomailbox.ch>
Cc:     Simon Wunderlich <sw@simonwunderlich.de>,
        Antonio Quartulli <a@unstable.cc>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        b.a.t.m.a.n@lists.open-mesh.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, lvc-project@linuxtesting.org
References: <20230526161632.1460753-1-VEfanov@ispras.ru>
 <5834562.MhkbZ0Pkbq@bentobox>
From:   Vlad Efanov <vefanov@ispras.ru>
In-Reply-To: <5834562.MhkbZ0Pkbq@bentobox>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Sven,


cancel_delayed_work_sync() and queue_delayed_work()

use WORK_STRUCT_PENDING_BIT in work->data to synchronize.

INIT_DELAYED_WORK() clears this bit.


The situation is :  __cancel_work_timer() sets WORK_STRUCT_PENDING_BIT

but INIT_DELAYED_WORK() in batadv_dat_start_timer() clears it

and queue_delayed_work() schedules new work.


Best regards,

Vlad.

On 26.05.2023 19:49, Sven Eckelmann wrote:
> On Friday, 26 May 2023 18:16:32 CEST Vladislav Efanov wrote:
>> The reason for these issues is the lack of synchronization. Delayed
>> work (batadv_dat_purge) schedules new timer/work while the device
>> is being deleted. As the result new timer/delayed work is set after
>> cancel_delayed_work_sync() was called. So after the device is freed
>> the timer list contains pointer to already freed memory.
> You are most likely right but could you please point out what in the worker is
> checked by the workqueue code that prevents it from being scheduled again?
> (and which seems to be overwritten as your patch seems to suggest)
>
> I think __cancel_work_timer marked the work as canceling but
> batadv_dat_start_timer reinitialized the worked (thus removing this important
> state). Would be nice if you could either correct me or confirm what I think to
> remember.
>
> Kind regards,
> 	Sven