| Message ID | 20210222151247.24534-4-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for skb with sk ref cloning | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: markus.theil@tu-ilmenau.de; 1 maintainers not CCed: markus.theil@tu-ilmenau.de |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 12 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, 2021-02-22 at 16:12 +0100, Oleksij Rempel wrote: > This code is trying to clone the skb with optional skb->sk. But this > will fail to clone the skb if socket was closed just after the skb was > pushed into the networking stack. Which IMHO is completely fine. If we then still clone the SKB we can't do anything with it, since the point would be to ... send it back to the socket, but it's gone. Nothing to fix here, I'd think. If you wanted to get a copy back that gives you the status of the SKB, it should not come as a huge surprise that you have to keep the socket open for that :) Having the ACK skb will just make us do more work by handing it back to skb_complete_wifi_ack() at TX status time, which is supposed to put it into the socket's error queue, but if the socket is closed ... no point in that. johannes
On 22.02.2021 17:30:59, Johannes Berg wrote: > On Mon, 2021-02-22 at 16:12 +0100, Oleksij Rempel wrote: > > This code is trying to clone the skb with optional skb->sk. But this > > will fail to clone the skb if socket was closed just after the skb was > > pushed into the networking stack. > > Which IMHO is completely fine. If we then still clone the SKB we can't > do anything with it, since the point would be to ... send it back to the > socket, but it's gone. Ok, but why is the skb cloned if there is no socket linked in skb->sk? | static u16 ieee80211_store_ack_skb(struct ieee80211_local *local, | struct sk_buff *skb, | u32 *info_flags, | u64 *cookie) | { | struct sk_buff *ack_skb; | u16 info_id = 0; | | if (skb->sk) | ack_skb = skb_clone_sk(skb); | else | ack_skb = skb_clone(skb, GFP_ATOMIC); Looks like this is dead code, since both callers of ieee80211_store_ack_skb() first check if there is a skb->sk | if (unlikely(!multicast && ((skb->sk && | skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS) || | ctrl_flags & IEEE80211_TX_CTL_REQ_TX_STATUS))) | info_id = ieee80211_store_ack_skb(local, skb, &info_flags, | cookie); > Nothing to fix here, I'd think. If you wanted to get a copy back that > gives you the status of the SKB, it should not come as a huge surprise > that you have to keep the socket open for that :) > > Having the ACK skb will just make us do more work by handing it back > to skb_complete_wifi_ack() at TX status time, which is supposed to put > it into the socket's error queue, but if the socket is closed ... no > point in that. We haven't looked at the callers of ieee80211_store_ack_skb(). Marc
On Mon, 2021-02-22 at 19:51 +0100, Marc Kleine-Budde wrote: > On 22.02.2021 17:30:59, Johannes Berg wrote: > > On Mon, 2021-02-22 at 16:12 +0100, Oleksij Rempel wrote: > > > This code is trying to clone the skb with optional skb->sk. But this > > > will fail to clone the skb if socket was closed just after the skb was > > > pushed into the networking stack. > > > > Which IMHO is completely fine. If we then still clone the SKB we can't > > do anything with it, since the point would be to ... send it back to the > > socket, but it's gone. > > Ok, but why is the skb cloned if there is no socket linked in skb->sk? Hm? There are two different ways to get here, one with and one without a socket. johannes
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 5d06de61047a..c0dd326db10d 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -2439,11 +2439,7 @@ static u16 ieee80211_store_ack_skb(struct ieee80211_local *local, struct sk_buff *ack_skb; u16 info_id = 0; - if (skb->sk) - ack_skb = skb_clone_sk(skb); - else - ack_skb = skb_clone(skb, GFP_ATOMIC); - + ack_skb = skb_clone_sk_optional(skb); if (ack_skb) { unsigned long flags; int id;
This code is trying to clone the skb with optional skb->sk. But this will fail to clone the skb if socket was closed just after the skb was pushed into the networking stack. Fixes: a7528198add8 ("mac80211: support control port TX status reporting") Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- net/mac80211/tx.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)