Received-SPF: pass (google.com: domain of linux-kernel+bounces-22058-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <5a302c81-427e-4529-9250-5596a740964f@virtuozzo.com>
Date: Wed, 10 Jan 2024 19:16:35 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] neighbour: purge nf_bridged skb from foreign device neigh
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>
 <07490c75-86c3-4488-8adb-7740b14feb30@virtuozzo.com>
 <20240109111228.GA7664@breakpoint.cc>
Content-Language: en-US
From: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
In-Reply-To: <20240109111228.GA7664@breakpoint.cc>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?UmdYeU5ka2k2U1A0Yi9JdXVHeXR4bWRrVDRYbThyd0ptQ0tFLzlvTlpvQXNO?=
 =?utf-8?B?RU5Ycnpzd2xnUU9VSzNvVm1pZHZqWHU4dmtmSHAyNC9LMDA3L2ZBYWNkTmd1?=
 =?utf-8?B?R0ZZZmJIaXV0eWd5cnRmZXBsUFhoVG1ZZlVQaTYzQ1hvK1Y0aGliOXcyZnR6?=
 =?utf-8?B?Y011akF4UUhwSk1wUUQ3Rm94ZHovQlNzenNLU0dTWlFKTE9lb0hQU0tsdU9W?=
 =?utf-8?B?ZkVlUFowOGYvMk1XWXREQzNqQXBtUHNhMUJUNEpPL0dtb0R2SWpQcHlJYzdI?=
 =?utf-8?B?UEg5YzhVa1VIRTFhS3MvQitZK0R3U044em5Wa2JsWlQ2ZWVxUVRWbEhCU0Zp?=
 =?utf-8?B?R256OXFYYnM0TEllWnVsT3MvZFZvOFptWGYvMUtYUUxmMFlVQ2gvMDZ4c09T?=
 =?utf-8?B?aWpGREZZa21mL2N5elJuRllDbnFSeVZZN25sNVBaUVlpWEplNExGRjVJUW5T?=
 =?utf-8?B?MldubXozWnRwUUc1Zy8xUVZOcGUyL3VjSHZ0dGdXenExSlB4NjI0R3FEZDly?=
 =?utf-8?B?U082R2g1N0JXVGZKREFJVlcrRzJsdTRnMTNXcFhFSEw2QnB6K0VidU1iWnZo?=
 =?utf-8?B?V0lzYWUwL3FUSys3RVhDVXNGQzI2OVNkVy9vazA2aExCLzFtbEZSeWNNWFNU?=
 =?utf-8?B?TUh0ZXc3SHFhQzAwem5ia2cwUUFWY3d6Q2lOVDByZkpEWCtCWWNRTER2cmFM?=
 =?utf-8?B?S0xVOUVPMmk4UWkxWExrcHowRVozVmltcUFHRUh0Z0ROdEF5czhMSTFFL1ox?=
 =?utf-8?B?M09xdkp3aEpJZlZicnJuTE1IUnpLV3puRzhMMDk5ZlFIcWUvcjV0L0NUVFdB?=
 =?utf-8?B?NXdPTkZrWkFtbVBsUlBFQXhPY3F6dElrTFdYcVZKOFZRaEFrc1FTd3MvblRG?=
 =?utf-8?B?ckhsZms4VWFTWm94Tm1yd05mRmZKQXhPV2h4THR5YzFSaCtBYjBKckwxV2ty?=
 =?utf-8?B?WWMxWUIxQmlvdmwvajlsYVFScUpHOFNPV2g5aGt2Q3hyMGdhaTFsdjBEK2JG?=
 =?utf-8?B?TmdyQzJDK1UwaWUrQWZRbjhrRVpSWFBNS2FjNCs4UDlRRW9ydGpYVFJ0OVBo?=
 =?utf-8?B?OUJKMlMyTVhkV280YkNiSkZieklTdTJLckgzb3F4MW1tcmJtYjJaT1VFVnZr?=
 =?utf-8?B?dWFQWTFDOFpuTTFldVZNWUFvSGFwZ29sOGpnU1ZldFExb0tQT2MrV0YwVVc4?=
 =?utf-8?B?SThjenhCSkJaU1RqN1VlR0lOSklYdlBYcUgwcFR5SUtTS21OLzVWM3JYU3NQ?=
 =?utf-8?B?QVhyZk1GVGhWYW1hL0ZDWUxFSW4rMjg4UkhxVFlOMjlpYUlzaDduak9BS1kr?=
 =?utf-8?B?V0dMcnEwSUtXeWVFa3JYS1FpUFhEbm1JS20zOS9zTHl1VlhmV2VESlhQUEpB?=
 =?utf-8?B?bGpud2xQTHRQbWtYaXZmUEt0RHZOZjc5YU5XaUZqUjEvSEJhZkZtU2x3aVl3?=
 =?utf-8?B?dXlJbnZTUU1pTm5wb3FGeldQV3JRbHFydmFSVWQ4ZnM4bVFJckRJQUV5c2Jh?=
 =?utf-8?B?M2k1Z0tlQloxOXZZcEVpZnZma0gwMlNXdWxCdnVVTGpIYTFZc0JxUlo4ZjNn?=
 =?utf-8?B?U3diSURtQVo2OCtRdTJ4TDgwcTR3eHRJcnBCM1RTSHU4RGgzbHQ2bEZBZUtz?=
 =?utf-8?B?ek5lYVNBT0JvNE1DaGRMRWpldk0wcjNnTVNNUXZZNVNpOEpYM0grNm0wSXZK?=
 =?utf-8?B?SUJndHdpclVYTEZJZi84Y3Q5QWxQSTJYWmNXZEdSMWpiYUFTMjIwK1owM0tz?=
 =?utf-8?B?NzlOTXdIZnVUZERYdjFza2ttNytzZkovdThUQ1ZJZEtlNGJBaHVpbHlHdi9x?=
 =?utf-8?B?VWxFSEJrcnBjYlJKYVo2aS9XaW5Ocm9WTEZRN3Z0MVBwSFQwdG5jbjU4L3Rq?=
 =?utf-8?B?cG5pL3pJcWNsaWxnOVlGZFgzU2x0TUlCaHVKTGk1T2hudS9yaFRVZ08zUWNU?=
 =?utf-8?B?WllEMDRWQ2pMajlONUI4a3YvSVB0K21PT3pmVGlOOTVmdFhZZEFwSDN4MlpP?=
 =?utf-8?B?ZmpudDNVTDZMejdNcXVDTHpyWFdUSGI2OExBUkJDWUVJd2RiUFlid05TaFRF?=
 =?utf-8?B?S2JaVHFtU3lycEVBZVBqWEMxNTdXUlpNeDRIenFjM1E4cWdiQWQ4cjZnV2Np?=
 =?utf-8?B?Z2kwMWNVQkJUMTBsVGtGSnlxdWJjamxUSVFEakJBUS9RbFVxdlJkeUVGa0Nk?=
 =?utf-8?B?RC8vZG4yYTJTd2NDeGVFUVEvbWJVWXBvTWEzMWhRRmlSK3dQUjYxUFM5SkRy?=
 =?utf-8?B?dEpRdVE0S0YyZEY1L1VGUVlMTllRPT0=?=
X-OriginatorOrg: virtuozzo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e21ae203-fb76-4a4c-9f03-08dc11cda00b
X-MS-Exchange-CrossTenant-AuthSource: DU0PR08MB9003.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jan 2024 11:16:43.3554
 (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: ZALf2vc7v251gax5/QRVEiIIS0g/WQbpLHIrB7Jwbhjuq5+VhtURcdwoY5J4urAXfUfZuyMlBdrqQqGLkzXmj0o5gmsF3N5nxzRli07K0v0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU0PR08MB9558

Here is the new version 
https://lore.kernel.org/netdev/20240110110451.5473-1-ptikhomirov@virtuozzo.com/

On 09/01/2024 19:12, Florian Westphal wrote:
> Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote:
>> 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);
> 
> You could use dev_net(skb->dev), yes.

In br_nf_pre_routing_finish_bridge_slow I had to use dev_net(skb->dev).

> 
> Or create a preparation patch that does:
> 
> -nf_bridge_get_physindev(const struct sk_buff *skb)
> +nf_bridge_get_physindev(const struct sk_buff *skb, struct net *net)
> 
> (all callers have a struct net available).

For all other cases I did the prep-patch propagating net.

> 
> No need to rename the function, see below.
> 
>> -       br_indev = nf_bridge_get_physindev(oldskb);
>> +       rcu_read_lock_bh();
>> +       br_indev = nf_bridge_get_physindev_rcu(oldskb);
> 
> No need for rcu read lock, all netfilter hooks run inside
> rcu_read_lock().

Thanks for this hint! I have checked all those tons of cases and 
actually proved to myself that all cases have rcu_read_lock =)

> 
>> Does it sound good?
> 
> Yes, seems ok to me.
> 
>> 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.
> 
> Alternative would be to add a 'br_nf_unreg_serno' that gets incremented
> from brnf_device_event(), then store that in nf_bridge_info struct and
> compare to current value before net_device deref. If not equal, toss skb.
> 
> Problem is that we'd need some indirection to retrieve the current
> value, otherwise places like nfnetlink_log() gain a module dependency on
> br_netfilter :-(
> 
> We'd likely need
> const atomic_t *br_nf_unreg_serno __read_mostly;
> EXPORT_SYMBOL_GPL(br_nf_unreg_serno);
> 
> in net/netfilter/core.c for this, then set/clear the
> pointer from br_netfilter_hooks.c.
> 
> I can't say/don't know which of the two options is better/worse.
> 
> s/struct net_device */int// has the benefit of shrinking nf_bridge_info,
> so I'd try that first.

Ok, did s/struct net_device */int// variant.

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