Re: [PATCH v3 3/5] batman-adv: Do not let TT changes list grows indefinitely

[Remi Pommarel <repk@triplefau.lt>]

On Thu, Nov 21, 2024 at 02:50:18PM +0100, Antonio Quartulli wrote:
> On 20/11/2024 18:47, Remi Pommarel wrote:
> > When TT changes list is too big to fit in packet due to MTU size, an
> > empty OGM is sent expected other node to send TT request to get the
> > changes. The issue is that tt.last_changeset was not built thus the
> > originator was responding with previous changes to those TT requests
> > (see batadv_send_my_tt_response). Also the changes list was never
> > cleaned up effectively never ending growing from this point onwards,
> > repeatedly sending the same TT response changes over and over, and a
> > creating a new empty OGM every OGM interval expecting for the local
> > changes to be purged.
> > 
> 
> nice catch!
> 
> > When there is more TT changes that can fit in packet, drop all changes,
> > send empty OGM and wait for TT request so we can respond with a full
> > table instead.
> > 
> > Fixes: e1bf0c14096f ("batman-adv: tvlv - convert tt data sent within OGMs")
> > Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> > ---
> >   net/batman-adv/translation-table.c | 19 ++++++++++++++-----
> >   1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
> > index bbab7491c83f..d7b43868b624 100644
> > --- a/net/batman-adv/translation-table.c
> > +++ b/net/batman-adv/translation-table.c
> > @@ -990,6 +990,7 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv)
> >   	int tt_diff_len, tt_change_len = 0;
> >   	int tt_diff_entries_num = 0;
> >   	int tt_diff_entries_count = 0;
> > +	bool drop_changes = false;
> >   	size_t tt_extra_len = 0;
> >   	u16 tvlv_len;
> > @@ -997,21 +998,29 @@ static void batadv_tt_tvlv_container_update(struct batadv_priv *bat_priv)
> >   	tt_diff_len = batadv_tt_len(tt_diff_entries_num);
> >   	/* if we have too many changes for one packet don't send any
> > -	 * and wait for the tt table request which will be fragmented
> > +	 * and wait for the tt table request so we can reply with the full
> > +	 * table.
> 
> I'd still say "(fragmented) table", in order to give the reader a clue about
> how we're going to handle this.
> 
> > +	 *
> > +	 * The local change history should still be cleaned up or it will only
> > +	 * grow from this point onwards. Also tt.last_changeset should be set
> > +	 * to NULL so tt response could send the full table instead of diff.
> 
> Personally I'd not add these details.
> I'd just say that the history "should still be cleaned up as we get ready
> for the next TT round". Or something along those lines.
> The rest is just a consequence of the "preparation".
> 
> >   	 */
> > -	if (tt_diff_len > bat_priv->soft_iface->mtu)
> > +	if (tt_diff_len > bat_priv->soft_iface->mtu) {
> >   		tt_diff_len = 0;
> > +		tt_diff_entries_num = 0;
> > +		drop_changes = true;
> > +	}
> >   	tvlv_len = batadv_tt_prepare_tvlv_local_data(bat_priv, &tt_data,
> >   						     &tt_change, &tt_diff_len);
> >   	if (!tvlv_len)
> >   		return;
> > -	tt_data->flags = BATADV_TT_OGM_DIFF;
> > -
> > -	if (tt_diff_len == 0)
> > +	if (!drop_changes && tt_diff_len == 0)
> >   		goto container_register;
> > +	tt_data->flags = BATADV_TT_OGM_DIFF;
> 
> hmm there is no mention in the commit message as to why we're moving this
> assignment. Why is that?

No reason, the if is supposed to be after this flag, thanks.

> 
> [And I just saw that this flag is never used.......]
> 

Thanks for the review.

-- 
Remi