Received-SPF: pass (google.com: domain of linux-kernel+bounces-20388-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <07490c75-86c3-4488-8adb-7740b14feb30@virtuozzo.com>
Date: Tue, 9 Jan 2024 12:57:36 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] neighbour: purge nf_bridged skb from foreign device neigh
Content-Language: en-US
From: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
To: Florian Westphal <fw@strlen.de>
Cc: "David S. Miller" <davem@davemloft.net>,
 Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
 Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, kernel@openvz.org
References: <20240108085232.95437-1-ptikhomirov@virtuozzo.com>
 <20240108111504.GA23297@breakpoint.cc>
 <a84b2797-2008-45d6-9ca3-c72666d3c419@virtuozzo.com>
In-Reply-To: <a84b2797-2008-45d6-9ca3-c72666d3c419@virtuozzo.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Y0lrUG94N3FPam5Wb2Q3Q0JIOC9UYzd0UVpYbTBqWXNFUnBONWoyUjNEWU5m?=
 =?utf-8?B?ZCtJdkJ6WWFESHdteXBrcU9OMVBRYmRlTlRBOGt1M1lKVU4zRnVkRTlleHpW?=
 =?utf-8?B?c3pLRU82bEpvRWhjblFkWjJxLyttdTNPZkhXdmlIV011bVIySGJKRlltTjFN?=
 =?utf-8?B?MkdnZnU4UWdjVkJwd2ltb0NnKytkL3E4NXVBUVc2UWNZQjJhOXRNN0wwT1Z4?=
 =?utf-8?B?K29Fc2RUeG44YXN3d3J4YmhQK09ZQ21BcStkMGc0ZjNIV0crd1RJODY4TVNw?=
 =?utf-8?B?VXhaeWhibWFrK0pKR09pekYxWXpvaWpPOHY4OGIrQnFtWGIwYksreWpQZUZi?=
 =?utf-8?B?TXF5QXg5YmVWWUp6dmpwZGxyWWxsbEpsa2d3bjNLQmZIbm5Sb1ZLcE40amlC?=
 =?utf-8?B?S2djODE5d2lYbnV5WUQ3QjNSVTVCT2RjT0tzUmlZSlVRL1VocXArNk1BaTZJ?=
 =?utf-8?B?YW9OVlhteUkyejZIK0tkZitKVDRKWlFqSWVqMW1XaWVucHJNZmlTMEFaVnVI?=
 =?utf-8?B?dTdGZFRjUVBySUhpM08wcXdWaHhWVWJ1V213emhqeUZLVjNha0JnVm4xdFow?=
 =?utf-8?B?L0pDc1dKNEQ2R3JWY25GaWV4cFR1eFJQZ0s5K1ZBd1g0Sm9lODlpaFB5NTE0?=
 =?utf-8?B?a1Jnd3llOU9iRno2c1BRcE5QMmdmWFBraC9KZWM4QVpWWG4rQ3NDSEdVMW8v?=
 =?utf-8?B?c0MrcWY3RVJpd2F2Z1RhTldnOTVldkdPZStaYTBHZ0hJRGpHMkdDSGdXMTkx?=
 =?utf-8?B?bEFPTE9oQUIzZWx3TDErTDFFV2o2bEFaNlN1dVBINllhNXozRGJsSUVDeGNH?=
 =?utf-8?B?SXNSSnh0ZWxmQ2FKMjk4SWJzL28yRFBlRkdHWHVlanJFVDJBcE4rL3M4aFRP?=
 =?utf-8?B?L0h3NjZMaHZPdXBqck9jNklCSTB2aEd2VkRiK0JpdGQ0eXNZckFRckxjMDdO?=
 =?utf-8?B?RHBqejlFS09HWU1IZDMrcElpbmpLTnN5ZzNWZDg1QXp0dmp1dG84L2l4eE5I?=
 =?utf-8?B?WVJMS1RDaU1Idm1SWmtMcVRyZW9HVWN0dXdxWnhYOUVHczA0ZkcyZHJHenBV?=
 =?utf-8?B?cmhVL0wzTlFoNWpocjBlVlRwQUxEWFJ5ZVFPc0lpSlNMUFNHelRYL1BGclkz?=
 =?utf-8?B?ODVId1dzT1F3N00yWkt2NStDY2dFNXZPa09zVEtTdWdvOUk0amdGT3ozV3pp?=
 =?utf-8?B?cHIzZy82TjlvdmhnZWlUTml0d1hhajFqNlNTQWhJU2gxNStXazdFenc4VHNt?=
 =?utf-8?B?SzVDdm5WNi8yN09qMjJoMkhybXVaQ21ZUDhhL0pxbUNzeW16Szh2dmVmWi9l?=
 =?utf-8?B?OEpEc3ZEOEhGYWRjc2pkKzBHZENpZHNqRHl3RXlUcllqWitTNTZkN2ZNLzFU?=
 =?utf-8?B?UDMxVzJXcUgxNmk1bjRKU3RhUXVMK1F0YnFkcXFuVnUyeDZsQXB1V29BRXZL?=
 =?utf-8?B?MEs0cGovZUs4cUxtd2Q2RDBYT0lna0c5RkdhdVphYktkWmxSZ1liRUdvVS94?=
 =?utf-8?B?MUVYWEhDbWNmY3Z3cXNYRnRzMEM1aUN4TStKYis2c0d5QXc3ZklZUE1udDdB?=
 =?utf-8?B?WXViNk1OWVMxbGNXSWtMcm1lN1NpMGxXRHlabjRQbFFWd05BeUREZGlGMDMr?=
 =?utf-8?B?Rkh0QkZyMzFqcWkxVDJqOUphNnFwcXVNNms5OE03R3pxUGVpUVREdC9KOE8v?=
 =?utf-8?B?emIrUlNHeldEbWVNUG83Y0VZUWYvMUxzNUo5SlhNSjA2Wm1MUnRxRXdKMXZR?=
 =?utf-8?B?OWtPR0p4ditnUk11RmZ1Vk5MMGVHQnlyb0prQndPcGYvL1Fad2E2MWFDNmRK?=
 =?utf-8?B?dHNuQWlwVy90bS9UenVoekp3RFU4NkpPWjVzcmYwVDhWUmUrSk5ybHFRd3NV?=
 =?utf-8?B?QlpNV2dkcHdTVE1CZWhCZml1WW5iUnNGbWF5OCtGaU1XM3pnd2JPS21TaXRY?=
 =?utf-8?B?WGp0WkNudDU5VlpaNDBIMDdKRVNkYmRVM3kwdWRGaWUrdFV3eERHaTU4ZEtY?=
 =?utf-8?B?Y3lsSW01ZGVseWI2Y3hhMHpWTjdwd1hMNUg3K0VhaWZPaHEyR1FYZTlWSHhX?=
 =?utf-8?B?ZDE0VEwxL2RPaERMVWJCVEQ0c1NqOW96aG1TYXhyRkhIOUd1ZFpBNGR6QmhR?=
 =?utf-8?B?K3QrNGFCaEp5WUxILzVueXJkN3lsbGRNRFRGN3k4NzdNS0RGRjhlbkZpUFRU?=
 =?utf-8?B?OVFEMlJLdmxvU282VDdBZ2FieFQzNTNKRzVKelM3aHJITThqQ2trTGlFSjRG?=
 =?utf-8?B?Y3BYQWpxWE92cVVyd3JSc3JJck1RPT0=?=
X-OriginatorOrg: virtuozzo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4e75c128-3fde-45cc-555e-08dc10cf843f
X-MS-Exchange-CrossTenant-AuthSource: DU0PR08MB9003.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jan 2024 04:57:44.5697
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xpznPKVfWDSCzhNnkpkYJGclaTrsdYbbZgkmxWMZOjN/FDpa7NxRMmzuG8sxpqvPRYBRk40uuOvEcUqrHS82asoc+4Dfzl2Sci0kXbh+gmw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB9PR08MB8410


On 08/01/2024 19:26, Pavel Tikhomirov wrote:
> 
> 
> On 08/01/2024 19:15, Florian Westphal wrote:
>> Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote:
>>> An skb can be added to a neigh->arp_queue while waiting for an arp
>>> reply. Where original skb's skb->dev can be different to neigh's
>>> neigh->dev. For instance in case of bridging dnated skb from one veth to
>>> another, the skb would be added to a neigh->arp_queue of the bridge.
>>>
>>> There is no explicit mechanism that prevents the original skb->dev link
>>> of such skb from being freed under us. For instance neigh_flush_dev does
>>> not cleanup skbs from different device's neigh queue. But that original
>>> link can be used and lead to crash on e.g. this stack:
>>>
>>> arp_process
>>>    neigh_update
>>>      skb = __skb_dequeue(&neigh->arp_queue)
>>>        neigh_resolve_output(..., skb)
>>>          ...
>>>            br_nf_dev_xmit
>>>              br_nf_pre_routing_finish_bridge_slow
>>>                skb->dev = nf_bridge->physindev
>>>                br_handle_frame_finish
>>>
>>> So let's improve neigh_flush_dev to also purge skbs when device
>>> equal to their skb->nf_bridge->physindev gets destroyed.
>>
>> Can we fix this by replacing physindev pointer with plain
>> ifindex instead?  There are not too many places that need to
>> peek into the original net_device struct, so I don't think
>> the additional dev_get_by_index_rcu() would be an issue.
> 
> I will work on it, thanks for a good idea!
> 

If we replace nf_bridge->physindev completely, we would need to do 
something like this in every place physindev was used:

diff --git a/include/linux/netfilter_bridge.h 
b/include/linux/netfilter_bridge.h
index f980edfdd2783..105fbdb029261 100644
--- a/include/linux/netfilter_bridge.h
+++ b/include/linux/netfilter_bridge.h
@@ -56,11 +56,15 @@ static inline int nf_bridge_get_physoutif(const 
struct sk_buff *skb)
  }

  static inline struct net_device *
-nf_bridge_get_physindev(const struct sk_buff *skb)
+nf_bridge_get_physindev_rcu(const struct sk_buff *skb)
  {
         const struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
+       struct net_device *dev;

-       return nf_bridge ? nf_bridge->physindev : NULL;
+       if (!nf_bridge || !skb->dev)
+               return 0;
+
+       return dev_get_by_index_rcu(skb->dev->net, nf_bridge->physindev_if);
  }

  static inline struct net_device *
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a5ae952454c89..51e7cdf9b51c9 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -295,7 +295,7 @@ struct nf_bridge_info {
         u8                      bridged_dnat:1;
         u8                      sabotage_in_done:1;
         __u16                   frag_max_size;
-       struct net_device       *physindev;
+       int                     *physindev_if;

         /* always valid & non-NULL from FORWARD on, for physdev match */
         struct net_device       *physoutdev;
diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c 
b/net/ipv4/netfilter/nf_reject_ipv4.c
index f01b038fc1cda..01b3eb169772e 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -289,7 +289,8 @@ void nf_send_reset(struct net *net, struct sock *sk, 
struct sk_buff *oldskb,
          * build the eth header using the original destination's MAC as the
          * source, and send the RST packet directly.
          */
-       br_indev = nf_bridge_get_physindev(oldskb);
+       rcu_read_lock_bh();
+       br_indev = nf_bridge_get_physindev_rcu(oldskb);
         if (br_indev) {
                 struct ethhdr *oeth = eth_hdr(oldskb);

@@ -297,12 +298,19 @@ void nf_send_reset(struct net *net, struct sock 
*sk, struct sk_buff *oldskb,
                 niph->tot_len = htons(nskb->len);
                 ip_send_check(niph);
                 if (dev_hard_header(nskb, nskb->dev, ntohs(nskb->protocol),
-                                   oeth->h_source, oeth->h_dest, 
nskb->len) < 0)
+                                   oeth->h_source, oeth->h_dest, 
nskb->len) < 0) {
+                       rcu_read_unlock_bh();
                         goto free_nskb;
+               }
                 dev_queue_xmit(nskb);
-       } else
+               rcu_read_unlock_bh();
+       } else {
+               rcu_read_unlock_bh();
  #endif
                 ip_local_out(net, nskb->sk, nskb);
+#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+       }
+#endif

         return;

Does it sound good?

Or maybe instead we can have extra physindev_if field in addition to 
existing physindev to only do dev_get_by_index_rcu inside 
br_nf_pre_routing_finish_bridge_slow to doublecheck the ->physindev link?

Sorry in advance if I'm missing anything obvious.

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.