Message-ID: <4ed8cace-3b2a-8cb8-43d9-bba1c342a3a5@oracle.com>
Date:   Thu, 9 Dec 2021 15:26:45 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.2
Subject: Re: [PATCH] tun: avoid double free in tun_free_netdev
Content-Language: en-CA
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Stephen Hemminger <stephen@networkplumber.org>,
        davem@davemloft.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1638974605-24085-1-git-send-email-george.kennedy@oracle.com>
 <YbDR/JStiIco3HQS@kroah.com>
 <022193b1-4ddd-f04e-aafa-ce249ec6d120@oracle.com>
 <20211208083614.61f386ad@hermes.local>
 <b141489b-780c-1753-2a83-ccb60c4554d0@oracle.com>
 <20211208155838.24556030@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
From:   George Kennedy <george.kennedy@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20211208155838.24556030@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TTB2Q2t5WUQ5czcwVk5RZlRKUGFTVzlwSks2UjFGZnE4a2cvMkZVSFVXbnRT?=
 =?utf-8?B?YXY5eldiUy9KSjVqNVZXbEZtdHUvYXlNTXpSZEZBREJSVVdYRTJlbzEwVWNx?=
 =?utf-8?B?cldUOGR1MHZUVnQ5TlVzOG5SYUxpMnlBSFY3b2sraTZ5M2VrT0tJWjRGS0Fo?=
 =?utf-8?B?UnIyYW92WGJvek9nc1A0WW9hMXkzcVVwUlVuNHFvLysrSFFpMlpkd29zdXZL?=
 =?utf-8?B?MjJpRm5mV3IrM3V0cmZMbytFSTRucTBsOVVUYXgwZ2lsRWtFa1Z5dWtDbzZI?=
 =?utf-8?B?R0dtOUk3dmFieVl5TjJrWkJxZG96bGs1RzM3ZjZEUEFmVUhXMjZsRGxHM1Na?=
 =?utf-8?B?bDVNb1dVTXVBenhGb3RZNkVwdXBBVkplSVNabTk2NjBtK0FueXorM0NsYkhG?=
 =?utf-8?B?MmFOWWlUcGVJZEVXVVBaeDFmclRNdFVXaWFLeTdrOC9KdE12Njg1UWdyM2th?=
 =?utf-8?B?TmhWZGk3RDdtUlZ0cFQwVWovN1FKVEN4SGRZUnlaaFRrSFZpa0JEanhNbWNo?=
 =?utf-8?B?ZzJGb0FBNG43Y1VSb3ZxQUJmZ0xCMHU5b2xPRlVidUh3dzVNWEdDcThCbWlo?=
 =?utf-8?B?RzRaMXV5NHRrUjRwV2w4enl3WFJ5V2duanE1T3ZUbjdCYnNUS0lGcTY0SVE2?=
 =?utf-8?B?ZVc1Vlk1S1ZLNkk1RlB5NzdWdnFQaEhEUW5xY0dxUit6NU9RNlIrbmo0VG1a?=
 =?utf-8?B?Zkc3UFNlc0xkWVY2NVdDSEtuNDVjbEdzODhmekFEMzNhc09yOXNUQXdZcVRL?=
 =?utf-8?B?c2VFTXFMUUhlSmpjVmJyaDhBTmVOemdnalZzQU5Pd3laWWJOTEdteTllZk9s?=
 =?utf-8?B?QnZuSU4wVTFEQnpUM2VSUmhNL2tMbnNPYS9qR1Z6VUhvbFNUci9IWXgxbFRr?=
 =?utf-8?B?bW9VTVhoMWVaZU5HSERoR0xDNHpiZmdaYkJaTTdOcVAraXE3RTBQSkwyUU81?=
 =?utf-8?B?Q1poYVhlY0hrRkJCWThrRDFwNjF4bzFaeUVVUHFwYVg2OWx2ZnlaUE1CNjNt?=
 =?utf-8?B?VVY0T08yWFhBRHlFZlBsRGtyMlQ4OEcra0tncDJHcHc4c2NLTjlRWW5YdzQx?=
 =?utf-8?B?OU1ERDF2WDZnSmhrek5yT0wvRThQQTg3RXkyTWtQQkdTbUF6cnZHeVdJU0Zk?=
 =?utf-8?B?SmFqbXg2U3RRSjZyOWZtVjlOSWJSOHFDek15WWRKdG5Ma21ZT01IczdaSGo3?=
 =?utf-8?B?b1hBYlFWSWRpbHRiRGtYNmpQc2t1L0FyU1FkK3lOVG42UmFHWUNKWUdnUHg4?=
 =?utf-8?B?TVRHN2crdzNGMFZxMXZxS3k5R3ZKSGNTdXhEbFN6RVdWOFBEeklOcFFIWC9B?=
 =?utf-8?B?Ry9RTGwyQUpCcS9hYUR1Tm5xZXJMZjJoWkxHZS90dlBLTHV3ckZTdmdzWU4z?=
 =?utf-8?B?VldISlk2cEVpczhwMHFWWXJPZndNMzY0N09Oa0hVQi9wM290Ynlwbnc5QVBx?=
 =?utf-8?B?WXZ3eFRlR0lKRVYybHdxVjd3K3lOR1poK2k2d1U0S3hWZy9PZURWcVNJbi9C?=
 =?utf-8?B?UHgvUGdqaXNWeXNBWmR2UHB6NU1TZzVVZ1dwei9oOXFPQUhNTU9SOGFyTXF6?=
 =?utf-8?B?N3VWbHFMQjNNQURYeTR4RWFVa2N3SWNEWk01UjJpYis2ckRWUTdmYnI1b3Ew?=
 =?utf-8?B?TkM2bGhwbmNqbitKaFFYRjRKRWFlcFp4RlZBRkcyYzdOOWlJY3VQRk9ZSng5?=
 =?utf-8?B?WkF5OUlOYm5BK2tLbVBUQngzVEVRYjhuenlISkkyRHRBYTlzbVRpMi94dVBB?=
 =?utf-8?B?QnhIVWNrTGpxTzFiem9FQkFjTHVXZVBaN2dsR3YwUHZKNTlSMGYxWDFQcE5x?=
 =?utf-8?B?dGh2SmNzUlBjS0x1QVJyY0pvUDRvUGh3cjVISmdlRjd6bmFrYUZpZnI5Q1NL?=
 =?utf-8?B?NjczTmJYRC9mNktkZmMxcjVGWjRwdVFHbXBsdUZ5MDVmOVMxMW94VDJ6SzBH?=
 =?utf-8?B?YVdhMnc2SDVZclRCazAvMlF4UXE4VFVUNEtEekZjOTVEa3VlclczbGREaUJi?=
 =?utf-8?B?MkRUU2xPWFpMSWVEdWVpUkI0Mm1rZlNqd2VWMEJmZHJYRWJxTWE3MnZkSGU2?=
 =?utf-8?B?WlhWKy9IUVcwQ3hPRkRkNzQwbzltVjRRTmZ3L0dMZWl3RHZ0dFg1RDhmb2F6?=
 =?utf-8?B?ekZBcHRLbFZEaFF0OWdEK1BCQ0l4bEovMzZ6VXpleWY1WkxlNzVqb0xSeHhh?=
 =?utf-8?B?d1NmTTAvQkNSaXJmVzFENDlZd0diYWhacUc2dEFld0pvMWxuWFh4aEdSTTdE?=
 =?utf-8?B?NHlaOElmV0V5d2wwckFqK25tbzl3PT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8713cb34-dc64-4c33-c046-08d9bb5239d6
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5192.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2021 20:26:48.6566
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: XRNcXBmtniykLyN5oIAUzyQZ6kjEsWc/nBOp5pP5+MCxnzKvAub3+LHHFsN/rMvtTGFkn3ZK+BT61lPcw6Hkbwuf2piHhIhO3hRtwpRdhks=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR10MB1635
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10193 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 spamscore=0
 phishscore=0 bulkscore=0 suspectscore=0 malwarescore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112090105
X-Proofpoint-ORIG-GUID: qNAm6tpE4du0Odoz-up-WTLWDcHiADKl
X-Proofpoint-GUID: qNAm6tpE4du0Odoz-up-WTLWDcHiADKl
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 12/8/2021 6:58 PM, Jakub Kicinski wrote:
> On Wed, 8 Dec 2021 11:44:02 -0500 George Kennedy wrote:
>>> It looks like a lot of the problem is duplicate unwind.
>>> Why does err_free_flow, err_free_stat etc unwinds need to exist if
>>> the free_netdev is going to do same thing.
>> Maybe instead do not call security_tun_dev_free_security(tun->security)
>> in err_free_flow if it's going to be done anyway in tun_free_netdev().
> That won't be good either. register_netdevice() has multiple failure
> modes, it may or may not call the destructor depending on where it
> fails. Either the stuff that destructor undoes needs to be moved to
> ndo_init (which is what destructor always pairs with), or you can check
> dev->reg_state. If dev->reg_state is NETREG_UNREGISTERING that means
> the destructor will be caller later.
>
> The ndo_init way is preferable, just cut and past the appropriate lines
> preceding registration into a ndo_init callback.
Thank you Jakub,

Looking at other ndo_init's to try and figure out how much before 
register_netdevice() should go in it.

George