Re: [PATCH v3 1/5] batman-adv: Do not send uninitialized TT changes

[Remi Pommarel <repk@triplefau.lt>]

On Thu, Nov 21, 2024 at 07:02:43PM +0100, Sven Eckelmann wrote:
> On Thursday, 21 November 2024 16:07:24 CET Remi Pommarel wrote:
> > So the patch would be quite similar, only tt->tt.changes_list_lock will
> > be taken sooner in batadv_tt_tvlv_container_update().
> > 
> > That will fix the ADD between two read situation as you described
> > though.
> > 
> > Do you still prefer this option ?
> 
> I can't speak for Antonio but I think I would prefer for the fix the current 
> version. The locking one would end up again with nested spinlocks and maybe 
> more refactoring. And it might be nicer for the stable backports to have less 
> noise in the patch.

I tend to second that, because if I understand correctly, even if
tt.changes_list_lock is held sooner in batadv_tt_tvlv_container_update()
the scenario Antonio described could still happen. Indeed if the ADD (or
even DEL for that matter) happens between VLAN table CRC computation
(batadv_tt_local_update_crc()) and the lock, neighbor will have CRC
mismatch and send TT_REQUEST anyway. The race window would be smaller
but still here.

So if I am not mistaken, the only solution to eliminate the race
completely would be to compute CRC while holding the lock, and this I
don't really like.

> 
> Btw. just noticed that the code (not in your change - but overall) for the 
> filling of diff entries could have been something like:
> 
> --- a/net/batman-adv/translation-table.c
> +++ b/net/batman-adv/translation-table.c
> @@ -980,6 +980,7 @@ static void batadv_tt_tvlv_container_update(struct 
> batadv_priv *bat_priv)
>  	int tt_diff_entries_count = 0;
>  	bool drop_changes = false;
>  	size_t tt_extra_len = 0;
> +	LIST_HEAD(tt_diffs);
>  	u16 tvlv_len;
>  
>  	tt_diff_entries_num = READ_ONCE(bat_priv->tt.local_changes);
> @@ -1011,9 +1012,10 @@ static void batadv_tt_tvlv_container_update(struct 
> batadv_priv *bat_priv)
>  
>  	spin_lock_bh(&bat_priv->tt.changes_list_lock);
>  	bat_priv->tt.local_changes = 0;
> +	list_splice_init(&bat_priv->tt.changes_list, &tt_diffs);
> +	spin_unlock_bh(&bat_priv->tt.changes_list_lock);
>  
> -	list_for_each_entry_safe(entry, safe, &bat_priv->tt.changes_list,
> -				 list) {
> +	list_for_each_entry_safe(entry, safe, &tt_diffs, list) {
>  		if (tt_diff_entries_count < tt_diff_entries_num) {
>  			memcpy(tt_change + tt_diff_entries_count,
>  			       &entry->change,
> @@ -1023,7 +1025,6 @@ static void batadv_tt_tvlv_container_update(struct 
> batadv_priv *bat_priv)
>  		list_del(&entry->list);
>  		kmem_cache_free(batadv_tt_change_cache, entry);
>  	}
> -	spin_unlock_bh(&bat_priv->tt.changes_list_lock);
>  
>  	tt_extra_len = batadv_tt_len(tt_diff_entries_num -
>  				     tt_diff_entries_count);
> 
> 
> And then you can also move this before "tt_diff_entries_num = ..." and 
> save the corresponding bat_priv->tt.local_changes for the spliced list to the 
> inside the lock also in a local variable. And then operate on this variable 
> for the other decisions. Of course, you must still clean the local list in 
> case of an error. Which of course would slightly change the behavior in case 
> of an allocation error in batadv_tt_prepare_tvlv_local_data (which would 
> previously kept the list as it was).
> 
> But if it would be done like this then we could also remove the READ_ONCE and 
> not introduce the WRITE_ONCE - just because local_changes is only touched
> inside a locked area (see changes_list_lock).
> 
> Please double check these statements - this was just a simple brain dump.

Yes that would be a much more elegant way to handle it. Unfortunately,
if I don't miss anything, the WRITE_ONCE/READ_ONCE would still be
needed because batadv_tt_local_commit_changes_nolock() has to load
tt.local_changes out of the lock to check if it needs to purge client
and recompute CRCs.

Thanks,

-- 
Remi