| Age | Commit message (Collapse) | Author |
|---|
|
Simon Wunderlich says:
====================
This feature/cleanup patchset includes the following patches:
- bla: avoid double free after failed backbone_hash alloc,
by Sven Eckelmann
- tt: update kerneldoc, by Sven Eckelmann (3 patches)
- tt: fix local DEL handling, by Sven Eckelmann (3 patches)
- tt: reject VLAN/TT entries before reaching size limit,
by Sven Eckelmann
- use assign_bit() where applicable, by Peng Fan
* tag 'batadv-next-pullrequest-20260930' of https://git.open-mesh.org/batadv:
batman-adv: use assign_bit() where applicable
batman-adv: tt: reject VLAN/TT entries before reaching size limit
batman-adv: tt: queue local DEL event before marking entry as pending
batman-adv: tt: queue local DEL event under bucket lock
batman-adv: tt: only queue local del event after successful unlink
batman-adv: tt: soften kernel-doc for batadv_tt_local_remove_now()
batman-adv: tt: clarify responsibility for roam flag during removal
batman-adv: tt: clarify kernel-doc for batadv_tt_global_purge_local
batman-adv: bla: avoid double free after failed backbone_hash alloc
====================
Link: https://patch.msgid.link/20260930094558.3723766-1-sw@simonwunderlich.de
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
Convert open-coded if/else with set_bit/clear_bit the assign_bit API.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The translation table is announcing its current state via the TT TVLV in
each OGM(2). If another originator detects a desync with its own copy of
this state information, it will request a full table sync. Each originator
must therefore be able to send a reply with its full table without hitting
the size limit of this response.
The translation table code already tries to limit the amount of local TT
entries. But the code itself fails to achieve this task because:
* the size check uses shared, unlocked information
* the size check isn't done for VLANs
When enough VLANs + TT entries are created in parallel, then the TT will
need more room then allowed for a full translation table reply. Other
originators will then send regularly requests for a table sync but never
get a reply.
To avoid this problem for new VLAN and local TT entries, each new entry
must reserve "room" before it is actually allocated. Only when enough
"room" is available, this reservation is granted. This makes it possible to
keep the reservation handling locked and therefore safe for multiple
parallel contexts.
For now, over-reservation caused by a reduction of the MTU is not taken
into account.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_tt_local_set_pending() originally queued the DEL change event and
only afterwards set BATADV_TT_CLIENT_PENDING on the local entry. Commit
976b159b3c12 ("batman-adv: tt: use protected flag modifications") inverted
this order for batadv_tt_local_remove() and batadv_tt_local_purge_list().
The hash bucket list_lock held around both steps does not help against
batadv_tt_local_add_existing() because it is not holding it.
CPU0 CPU1
flags |= ..._PENDING;
batadv_tt_local_add_existing()
flags &= ~..._PENDING;
batadv_tt_local_event(ADD)
batadv_tt_local_event(DEL)
If the ADD was already announced by a commit in between, the DEL is sent in
the next TTVN although the entry is no longer pending (after
batadv_tt_local_add_existing()) and the local entry was never purged. The
incorrect DEL will corrupt the CRC on neighbor nodes. A full table sync
request is therefore issued to resolve this problem.
When the DEL event is queued before the flag is set, a
batadv_tt_local_add_existing() which clears the flag afterwards queues its
ADD behind the DEL, and both cancel each other out.
But the check whether an entry has to be removed must not be separated (by
using two different critial flags_lock sections) from setting the flag (in
batadv_tt_local_event) either. Otherwise batadv_tt_local_add_existing()
could refresh the entry between both steps without queuing an ADD, and an
active client would be announced as removed and purged.
Fixes: 976b159b3c12 ("batman-adv: tt: use protected flag modifications")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=12
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_tt_local_remove() sets BATADV_TT_CLIENT_PENDING on an already
announced local entry and only afterwards queues the DEL change event. It
holds neither the hash bucket list_lock nor bat_priv->tt.commit_lock.
It can therefore be potentially interrupted in the middle:
CPU0 CPU1
batadv_tt_local_remove()
flags |= ..._PENDING;
batadv_tt_local_commit_changes()
..._purge_pending_clients()
hlist_del_rcu(&...->hash_entry);
batadv_tt_local_update_crc()
atomic_inc(&bat_priv->tt.vn);
batadv_tt_local_event()
/* DEL queued only now */
The client then disappears from the local table and from the CRC of the new
TTVN after batadv_tt_local_commit_changes() without a DEL change being
announced for it. Neighbours receiving the new CRC without previously
seeing the DEL will try to recover via a full table request.
Move the event into batadv_tt_local_mark_removed() and hold the bucket
list_lock of the entry around both the flag change and the DEL event to
avoid this scenario.
Fixes: 976b159b3c12 ("batman-adv: tt: use protected flag modifications")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=12
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
When batadv_tt_local_remove_now() finds another local TT entry with the
same MAC + VLAN, it will not try to remove it. A delete event must not be
queued because it would otherwise cause the queued ADD event for the other
TT entry to be dropped.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=8
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
Simon Wunderlich says:
====================
This cleanup patchset includes the following patches, all by Sven Eckelmann:
- translation table cleanups (12 patches)
- kerneldoc fixes noted by sashiko (3 patches)
* tag 'batadv-next-pullrequest-20260907' of https://git.open-mesh.org/batadv:
batman-adv: correct batadv_hash_remove kdoc return type
batman-adv: bat_iv: fix ogm_neigh_is_sob parameters references
batman-adv: tt: clarify kernel doc for batadv_tt_local_set_pending_event()
batman-adv: tt: drop the cleanup label from batadv_tt_local_remove()
batman-adv: tt: extract immediate purge of a local entry
batman-adv: tt: extract marking of a removed local entry
batman-adv: tt: extract removal of the superseded local entry
batman-adv: tt: replace add_orig_entry goto in batadv_tt_global_add()
batman-adv: tt: extract merging of flags into existing global entries
batman-adv: tt: extract allocation of new global entries
batman-adv: tt: extract update of dynamic client flags
batman-adv: tt: extract refresh of existing local entries
batman-adv: tt: replace forward gotos in batadv_tt_local_add()
batman-adv: tt: extract allocation of new local entries
batman-adv: tt: look up wifi state of incoming interface in helper
====================
Link: https://patch.msgid.link/20260907081824.2474040-1-sw@simonwunderlich.de
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
Cross-merge networking fixes after downstream PR (net-7.3-rc3).
Conflicts:
drivers/net/dsa/mt7530.c
3c18e3c9a54e ("net: dsa: mt7530: populate lpi_interfaces to fix EEE support")
10d9d8328e8a ("net: dsa: mt7530: replace mt7530_read with regmap_read")
Adjacent changes:
drivers/net/bonding/bond_alb.c
1746ef2e2df2 ("bonding: use skb_cow_head() in bond_do_alb_xmit() and rlb_arp_xmit()")
4cef95f72bbd ("bonding: fix u32 overflow in compute_gap()")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
The "never announced" statement is too harsh for an entry which could have
been actually be announced by a different context while the
batadv_tt_local_remove_now() is in the process of being called. "not yet"
might be more appropriate.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907081824.2474040-1-sw%40simonwunderlich.de?part=11
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
If an local client gets removed due to an roam, then the local entry must
be marked as "roamed" to handle the rerouting correctly. But this is not
done by batadv_tt_local_remove() directly but by the new helper
batadv_tt_local_mark_removed(). The former will only create similar flags
for the roaming event.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907081824.2474040-1-sw%40simonwunderlich.de?part=10
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The new kernel-doc didn't mention the "non-multicast clients" limitation.
It was also not clear that the roaming flag gets cleared when the
announcement didn't contain the roaming flag.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907081824.2474040-1-sw%40simonwunderlich.de?part=9
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
Not the data is returned after a batadv_hash_remove() but the pointer to
the hlist_node which was used to look up the entry in the hash.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=8
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
When batadv_bla_init() fails to initialize the backbone_hash, it is freeing
the (previously) allocated claim_hash. The initialization function will
then return an error and the net_device initialization will stop. The
destructor will be called instead and (indirectly via batadv_mesh_free() ->
batadv_bla_free()) will try to free the bat_priv->bla.claim_hash again.
The pointer must therefore be set to NULL after freeing it in the
initialization error path.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=6
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The batadv_iv_ogm_neigh_is_sob() kernel-doc summary line is still
referencing the old parameter names "neigh1" and "neigh2". But these are
now called "candidate" and "reference".
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=5
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The function batadv_tt_local_set_pending_event() is no longer setting the
BATADV_TT_CLIENT_PENDING because it must be done with the flags_lock held.
The kernel-doc must therefore correctly state that the flag must already be
set.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831135117.574836-1-sw%40simonwunderlich.de?part=12
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
This is another run of the Coccinelle script for converting kmalloc()
family of allocations to kmalloc_obj() via the existing rules in
scripts/coccinelle/api/kmalloc_objs.cocci
This catches both the set of kmalloc() uses added since the first
kmalloc_obj() conversions in v7.0 and adds a large group missed in the
first pass due to Coccinelle not interacting well with the cleanup.h
scoped_...() family of macros[1]. I worked around this with spatch's
"--macro-file" argument to a file with all the scoped_...() macros mapped
to Coccinelle's YACFE_ITERATOR[2] as that was the closest viable control
flow indicator I could find.
Build tested allmodconfig on x86, arm64, arm, loongarch, mips, powerpc,
riscv, and s390 with no new warnings.
Link: https://lore.kernel.org/lkml/202609021314.8A9C0B8@keescook/ [1]
Link: https://github.com/coccinelle/coccinelle/blob/master/standard.h [2]
Signed-off-by: Kees Cook <kees+treewide@kernel.org>
|
|
With both locked sections moved to their own helpers, the only thing left
in the cleanup section is the reference of the looked up entry. The "out"
label is therefore only reached with a valid entry, except for the lookup
failure at the very beginning.
Return BATADV_NO_FLAGS directly when there is no entry and let the two
remaining cases be an if/else.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
A client which was added after the last ttvn increment was never announced
and can be dropped from the local table right away. The code doing so is
the second half of batadv_tt_local_remove() and needed a goto to skip the
accounting when the entry was already gone from the hash.
Move it to batadv_tt_local_remove_now().
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The first locked section of batadv_tt_local_remove() does three things at
once
* it snapshots the flags for the return value
* it marks the client as ROAMed
* it decides whether the entry has to survive until the next ttvn increment
But this complex locked section is in the middle of the function and cannot
be written as scoped_guard() due to goto's in this routine.
Move it to batadv_tt_local_mark_removed() and report the decision via its
return value. But leave the batadv_tt_local_set_pending_event() flag
assembly outside the function because it has no locking requirements.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
A client which was announced by another originator is no longer a local
client. The code which drops it from the local table and takes over its
WIFI flag is the second cleanup stage of batadv_tt_global_add() and needed
its own goto to skip over the multicast case.
Move it to batadv_tt_global_purge_local().
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The "add_orig_entry" label was only used to skip the flag merging of the
non-temporary path. Which means that the code jumping to it and the code
below it are the two halves of an if/else.
Both halves now only decide whether the originator list has to be purged,
so turn them into an if/else and purge the list at a single place.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
When an already known global TT entry is announced again, its flags have to
be merged with the announced ones. Two of the merge steps also decide
whether the originator list has to be purged before the announced
originator is added.
Move this to batadv_tt_global_merge_flags() and let it report via its
return value whether the originator list has to be purged.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The code which allocates and initializes a brand new global TT entry makes
up a third of batadv_tt_global_add() and only needs the tt_addr, vid and
flags arguments. It is also the only part of the "new entry" branch which
does not deal with the global hash.
Move it to batadv_tt_global_create() and let it return the initialized
entry.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The WIFI and ISOLA flags are re-evaluated on every batadv_tt_local_add()
call, no matter whether a new entry was created or an existing one was
refreshed. It is mostly isolated from the rest of the
batadv_tt_local_add().
Move it to batadv_tt_local_update_flags(). To allow the caller to trigger
the queuing of ADD events, it returns whether one of the flags announced to
the other nodes actually changed.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
Local TT entries which have the BATADV_TT_CLIENT_NEW no longer set are
committed. The batadv_tt_local_size_inc() was called for them and thus the
batadv_tt_local_size_dec() has to be called also when
batadv_tt_local_remove() is called for them when the BATADV_TT_CLIENT_NEW
wasn't consumed in the remove path.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The handling of a client which is already part of the local translation
table is independent from the rest of batadv_tt_local_add(): it only
refreshes last_seen and evaluates the PENDING and ROAM flags of the entry
which was just looked up.
Move it to batadv_tt_local_add_existing(). It reports via its return value
whether the client has to be announced to the mesh again and whether the
client roamed back to its original location.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_tt_local_add() used two labels in the middle of the function to
express which parts of the common tail have to be executed: "add_event" to
announce a (re-)added client and "check_roaming" to skip that announcement.
Jumping forward into the middle of a function makes it hard to see which
path ends up sending an ADD event. Especially since the PENDING branch
reached "add_event" while the branch right below it reached "check_roaming"
instead.
Express the same in an "added" variable and turn the two client cases into
a plain if/else. The PENDING check returned early before the ROAM check
could run, so it becomes an "else if".
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The code which allocates and initializes a brand new local TT entry makes
up more than a third of batadv_tt_local_add(). It only shares the addr, vid
and iif_is_wifi parameters with the rest of the function.
Move it to batadv_tt_local_create(), which returns the fully initialized
entry. The error paths become plain returns.
The insertion into bat_priv->tt.local_hash intentionally stays in
batadv_tt_local_add(), keeping the extra reference for the hash next to the
error handling which has to drop it again.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_tt_local_add() only needs to know whether the client was seen on a
wifi interface. But it kept the net_device reference of the incoming
interface alive until its own cleanup section, together with a "net" and an
"in_dev" variable which are of no interest to the rest of the function.
Move the lookup to a small helper which acquires and releases the
net_device reference itself.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_tt_local_purge_pending_clients() decreases vlan->tt.num_entries for
every entry it unlinks, but an entry which still carries
BATADV_TT_CLIENT_NEW was never counted by batadv_tt_local_transition_new().
batadv_tt_local_resize_to_mtu() calls batadv_tt_local_purge() and
batadv_tt_local_purge_pending_clients() directly, without committing in
between, and it halves its timeout down towards zero. Once the timeout is
short enough, batadv_tt_local_purge_list() marks even freshly added clients
as pending. And each of them decreases a counter it never increased.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The batadv_tt_local_size_inc() must never be called for an entry which was
already removed from the list. Otherwise the removal from the hash cannot
correctly determine if the batadv_tt_local_size_dec() needs to be called or
not.
This assumption is broken by the use of rcu_read_lock() in
batadv_tt_local_transition_new() because it might still see entries in the
list which were already removed by a different context from the list. If it
then increments the size counter, nothing will reduce the counter again.
Simply because the removal (responsible for the decrement) already
happened.
Over the whole time, the actual hash list spinlock must be held when
transitioning NEW local entries to avoid list manipulations.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The flags of translation table entries are modified in various places using
RMW operations like:
* read flags + add flag + store
* read flags + remove flag + store
These were done without making sure that no other context is doing a
similar operation at the same time. If another context does modify the
flags then it could happen that a store of the flag modifications is simply
lost. This problem can usually be fixed at a later point when the flags are
tried to be adjusted again.
To reduce the time the wrong flags are used, it is better to use a TT entry
specific spinlock when accessing the u16.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
To use scoped_guard(), it is not allowed to use goto in the same routine.
These goto's should only be used for cleanups but are not necessary of
minimal helpers like batadv_is_my_client(),
batadv_tt_global_client_is_roaming() and
batadv_tt_local_client_is_roaming(). Removing the goto's is actually making
these functions more readable.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The function batadv_tt_local_set_flags() implements support for various
flags and to set/unset them. It is also possible to decide whether this
change should increase the TT size or not.
But in reality, this function is only used to transition tt local entries
from the NEW state and count these entries. Remove the rest of the code to
simplify the function.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The code for handling the roaming of clients is mostly self contained but
embedded in the already too large batadv_tt_local_add function. The
maintainability can be improved by keeping it in an separate function.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_hash_remove() searches a bucket with a compare callback and unlinks
the first matching entry. batadv_compare_tt() matches any entry for the
same MAC address and VLAN, not the object which was passed in, and the
callers in batadv_tt_local_remove() and batadv_tt_global_free() are not
serialized against the rest of the translation table in any way.
So when the looked up entry was already unlinked by another context and a
new entry for the same client was added in the meantime, these two
functions unlink that new entry instead.
Add batadv_compare_tt_entry(), which matches the very object which is
searched for, and use it for both removals. Nothing is unlinked when the
entry is gone already, batadv_hash_remove() then simply returns NULL and
only the reference of the calling context is dropped.
As a side effect the returned hlist_node can no longer belong to a
different object, so both functions can operate on the entry they were
given.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
An u8 array in structures have a natural alignment of only 1 byte. An
u8[ETH_ALEN] array must therefore be assumed to only be 1 byte aligned. But
many etherdevices.h functions are requiring a 2 byte (u16) alignment.
All these addresses were either on the start of a struct or after a struct
member which required at least an alignment of 2 byte. Still, annote all
u8[ETH_ALEN] arrays on the stack as __aligned(2) to make sure that this
assumption isn't broken by other modifications inside the structs.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
An u8 array on the stack has a natural alignment of only 1 byte. An
u8[ETH_ALEN] array on the stack must therefore be assumed to only be 1 byte
aligned. But many etherdevices.h functions are requiring a 2 byte (u16)
alignment.
Annote all u8[ETH_ALEN] arrays on the stack as __aligned(2) to make sure
that the compiler takes care of aligning it correctly.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260821094813.201800-1-sw%40simonwunderlich.de?part=5
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
neigh1 and neigh2 is not really helpful when looking at a code which needs
to judge the metric similarity between a candidate and a reference. This
already caused an implementation error in the B.A.T.M.A.N. V bonding code.
Simply using "reference" and "candidate" for the neighbors is a lot more
descriptive and makes it easier to understand the code.
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The neigh1 throughput must be more than 3/4 of the neigh2 throughput. Just
3/4 is not enough to return true.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_v_neigh_is_sob() implements the metric (throughput) similarity check
as:
neigh2 (the reference) has a better throughput than 3/4 of the neigh1
(candidate) throughput
But this results in a selection of bonding candidates in
batadv_find_router() which have a really low throughput. But the goal must
be to select candidates which have a good throughput.
The neigh1/neigh2 operands must be swapped in the comparison to use the
reference as such and select only neighbor+interface combinations with good
throughput.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
The _batadv_is_ap_isolated() is called by
* batadv_transtable_search()
- tt_local_entry is the source
- tt_global_entry is the destination
* batadv_is_ap_isolated
- tt_local_entry is the destination
- tt_global_entry is the source
It is therefore incorrect to document the tt_local_entry as sending client
and tt_global_entry as the destination client.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
If an unknown subtype of an unicast 4addr packets was detected in
batadv_dbg_arp(), it is printing the (known) packet_type instead of the
subtype. This must be switched to the subtype to have any kind of use for
the reader of the debug message.
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
Simon Wunderlich says:
====================
Here are a few batman-adv bugfixes:
- fix stale receive device on merged fragments, by Zhiling Zou
the others are written by Sven Eckelmann:
- bla: fix potential CRC corruption issues (2 patches)
- dat: avoid unaligned fault in IP extraction
- dat: atomically update mac addresses
- mcast: fix TX priority extraction for BATADV_FORW_MCAST
- mcast: fix skb sharing and linearization (2 patches)
- bla: fix freeing of claims on meshif deletion
* tag 'batadv-net-pullrequest-20260821' of https://git.open-mesh.org/batadv:
batman-adv: bla: fix freeing of claims on meshif deletion
batman-adv: mcast: linearize skbuff for packet generation
batman-adv: mcast: ensure unshared skb for multicast packets
batman-adv: fix TX priority extraction for BATADV_FORW_MCAST
batman-adv: dat: atomically update mac addresses
batman-adv: dat: avoid unaligned fault in IP extraction
batman-adv: bla: prevent CRC corruptions after claim flush
batman-adv: bla: avoid CRC corruption due to parallel claim add
batman-adv: fix stale receive device on merged fragments
====================
Link: https://patch.msgid.link/20260821094813.201800-1-sw@simonwunderlich.de
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
The network and transport header fields in struct sk_buff are 16-bit
offsets from skb->head, and U16_MAX is reserved as the unset transport
header value. batadv_tvlv_call_handler() sets both fields from a received
multicast TVLV without checking whether the TVLV end is representable.
If the end offset exceeds the field's range, skb_set_transport_header()
truncates it so that the transport header precedes the network header.
The negative difference is then returned by skb_network_header_len() as
a large u32. batadv_mcast_forw_packet() consequently accepts an oversized
multicast tracker and accesses memory beyond the skb data.
Add skb_set_transport_header_careful(), an offset-aware counterpart to
skb_reset_transport_header_careful(), which validates the final
head-relative offset before assigning it. Use the new helper in
batadv_tvlv_call_handler() and reject unrepresentable TVLVs before
setting the network header.
Fixes: 07afe1ba288c ("batman-adv: mcast: implement multicast packet reception and forwarding")
Cc: stable@vger.kernel.org
Signed-off-by: Kyle Zeng <kylebot@openai.com>
Co-developed-by: David Lee <david.lee@trailofbits.com>
Signed-off-by: David Lee <david.lee@trailofbits.com>
Acked-by: Sven Eckelmann <sven@narfation.org>
Link: https://patch.msgid.link/20260817084955.944189-1-david.lee@trailofbits.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
|
|
When the mesh interface is getting deleted, then
batadv_bla_del_backbone_claims() (via batadv_bla_purge_backbone_gw()) could
make sure that all claims gets removed. But this function is only executed
when bat_priv->bla.claim_hash is not NULL. And since batadv_bla_free() is
always setting it to NULL before it is (indirectly) called, it was never
actually executed.
But the batadv_bla_purge_claims() -> batadv_handle_unclaim() is at the
moment too fragile because the BLA code is not handling the rehashing in
batadv_bla_update_orig_address(). The stored backbone address doesn't have
to be the one actually used for the hash bucket selection during the
initial adding of the backbone. The batadv_handle_unclaim() can therefore
fail to find the respective backbone for the unclaim and then stop the
deletion.
But the actual backbone_gw object is not needed for the unclaim because all
relevant information is always provided by the caller. And the check for
the existence of the backbone_gw doesn't provide any additional security
check for the deletion of a claim.
Cc: stable@kernel.org
Fixes: 23721387c409 ("batman-adv: add basic bridge loop avoidance code")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_mcast_forw_packet() and batadv_mcast_forw_scrape() is not only
called (indirectly) by the unsharing+linearizing batadv_recv_mcast_packet()
handler. When it is called (indirectly) by batadv_mcast_forw_mcsend() then
it will be unshared but not linearized. The SKB_LINEAR_ASSERT() can
therefore cause a fatal BUG().
The linearization should happen during the expansion of the head because
the scrape function can be hit already during the initial
batadv_mcast_forw_mode() selection code:
* batadv_interface_tx
* batadv_mcast_forw_mode
* batadv_mcast_forw_mode_by_count()
* batadv_mcast_forw_push()
-> calls batadv_mcast_forw_expand_head() before everything else
* batadv_mcast_forw_push_tvlvs()
* batadv_mcast_forw_push_dests()
* batadv_mcast_forw_push_adjust_padding()
* batadv_mcast_forw_scrape()
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 90039133221e ("batman-adv: mcast: implement multicast packet generation")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
When a packet is transmitted via a batman-adv interface and has already
enough room for the header then nothing will make sure that the skbuff is
unshared. But it is not allowed to modify a currently shared skbuff.
Always make sure that the pskb_expand_head() is not only called for a too
small header but also for shared skbuffs.
Cc: stable@vger.kernel.org
Fixes: 90039133221e ("batman-adv: mcast: implement multicast packet generation")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
batadv_mcast_forw_mode_by_count() pushs the skb->data for BATADV_FORW_MCAST
forwarding via batadv_mcast_forw_mcsend(). But the
batadv_skb_set_priority() expects the ethernet header directly before
(skb->data + offset). With the moved skb->data, just some random data would
be accessed to get the priority data.
Move the batadv_skb_set_priority() before the decision about the handling
multicast packets and potential header modifications.
Cc: stable@vger.kernel.org
Fixes: 90039133221e ("batman-adv: mcast: implement multicast packet generation")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
|
When a MAC address is updated in batadv_dat_entry_add(), it is done using a
simple copy function. A parallel reader might only see parts of this
update. In worst case, the reader is transporting the half updated MAC
address over the network or is creating an ARP response using it -
poisoning the ARP cache.
atomic64_t can be used to store the 48 bit of a mac address. A reader will
then either see the old mac address or the new one - never a mixture of
both.
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 2f1dfbe18507 ("batman-adv: Distributed ARP Table - implement local storage")
Signed-off-by: Sven Eckelmann <sven@narfation.org>
|
