Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] net: mdiobus: Fix memory leak in __mdiobus_register
To:     Dongliang Mu <mudongliangabcd@gmail.com>,
        "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     Dan Carpenter <dan.carpenter@oracle.com>,
        Pavel Skripkin <paskripkin@gmail.com>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        andrew@lunn.ch, hkallweit1@gmail.com,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, p.zabel@pengutronix.de,
        syzbot <syzbot+398e7dc692ddbbb4cfec@syzkaller.appspotmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>
References: <20210928092657.GI2048@kadam>
 <6f90fa0f-6d3b-0ca7-e894-eb971b3b69fa@gmail.com>
 <20210928103908.GJ2048@kadam>
 <63b18426-c39e-d898-08fb-8bfd05b7be9e@gmail.com>
 <20210928105943.GL2083@kadam>
 <283d01f0-d5eb-914e-1bd2-baae0420073c@gmail.com>
 <f587da4b-09dd-4c32-4ee4-5ec8b9ad792f@gmail.com>
 <20210928113055.GN2083@kadam> <YVMRWNDZDUOvQjHL@shell.armlinux.org.uk>
 <20210928135207.GP2083@kadam> <YVM3sFBIHzEnshvd@shell.armlinux.org.uk>
 <CAD-N9QXHcrv+6SJSOz3NyBkAnze2wVf_azYmjbGCVd5Ge_OQng@mail.gmail.com>
From:   "Xu, Yanfei" <yanfei.xu@windriver.com>
Message-ID: <bc07380e-54ae-1699-aaaf-3380f11c3eaf@windriver.com>
Date:   Wed, 29 Sep 2021 10:05:42 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
In-Reply-To: <CAD-N9QXHcrv+6SJSOz3NyBkAnze2wVf_azYmjbGCVd5Ge_OQng@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NGVONUtmNUlIN3VsYjAvMk1WY3o0eTVzZkxuWnp4dHRrODVZVWl3UHdKODkz?=
 =?utf-8?B?NTNldkxDcldLczc1bXpMRk1vaVZZQ2xlbDFnWUxaRjZMdXRIeEt6UUptOUJM?=
 =?utf-8?B?d0hxRmxGY0RpQXBzTVZxUmd1TmZQLzMwNTV6dXM0SUYzZUNhNk5lUFZHVXlr?=
 =?utf-8?B?WTAxUzFkOEc1NVhhNEZPY0FwLzR4c3J1d2NFclJQUUJ1VFU5N05xb2d5ODg5?=
 =?utf-8?B?bjlLelVvaXhOMGRTR2prZ3RpY2ROUWhVdXJLZ2QvbFJIRVRGaW9sVW1pL2Nq?=
 =?utf-8?B?K0JFQWc4Smt1ZjhCOG9oZEoyN0xYSTBDSTNpWGJlZFQxTzhkYWJMZVg5anFG?=
 =?utf-8?B?cWd3NXFRRzBnNWZOWDIybURscDBYdlg3SEd0Qm4yS1J3YnRhMXpBNlBXaExz?=
 =?utf-8?B?SlhsbVBaL3ZCMTY2WWpMOEprV1k4SEROK2VxTUlMTm05RG9jT2lIWGVGQVRD?=
 =?utf-8?B?UGxoRU56UE5JcTBrQThMemZEeE9UcjRzankyQVUxNDdBR0JwdGl3Zjk1cXFk?=
 =?utf-8?B?UGF6dndwaGRBMGQ5c2ZqUGhRVjMzNEVSU1pqcFd1MWwvTkVtaGtPK3A1M1Jv?=
 =?utf-8?B?OGhaYnlwekFXOTFRUHJGTDRwZXFQUjRFdjJyQm5OT1pIcnkzYlJRc2ZEcnlZ?=
 =?utf-8?B?SFcwVTFwUHhQN3ZkdW1Eem96UlV1TFRINGVjcE9qWHcyZU9mSFhNc0Q5S2hl?=
 =?utf-8?B?MDIvRXprNnN0aFpVL1duckVnZDBpR3ZvWlZ3UHFXMkFPWDVReEkxcGlrMnUv?=
 =?utf-8?B?UXhoWVpaK2FzZGorVW4zUWkxeFQxQlFvS0UyNkx2SWxHZnVZUW9OK3ZmYlhN?=
 =?utf-8?B?eTBwNGk2VkxJS1d0aDl3Uy9rTnBRL2lpblBQRGoxUy9RSVRiTVc3SU1jNUk4?=
 =?utf-8?B?aTdocmFYWXE3VDA2emRMdjQ0RVdncE52TEtMdkFvUUNlSEgrMy80K1J4L012?=
 =?utf-8?B?VG00Ulpza3M1MWlPcUw3aXA4Z3VzMWxINHp4Y0ticmZsLy9KRWI2UDRsSUtX?=
 =?utf-8?B?UUdWaWtudUhqVW5oY3BNOUlIR1pZOVhZSk00WmlZdGlwVHZWVWNkcUtkQVps?=
 =?utf-8?B?T0dSL0pDQjZ5MElTT0N3a094M0Q3czk1a09TSFBjbFNpbFIzdTBZaHBFZjlV?=
 =?utf-8?B?dWJvZnlqb1ZEQi9kWkx5cE9aRDN3S3FnUUh0WmtBUy9ocUpMVkttcFRCZklq?=
 =?utf-8?B?TUVQRXNSNkNSS3kwTHUzRXhITHVLY05mR2djWWNzcndpSmxTUSt5K1pGT0ll?=
 =?utf-8?B?RFFzMEJndm5tNlAzVGduak9mYnlsMSsvbDBxeVlhNHJqNjJ5T01iRU1FZWFw?=
 =?utf-8?B?VnNoR0FiVXpPaHBTWGFiMC9IcEJ1NnNYTXNTaEdua0luVExxZ2ZuUzl4K2Qx?=
 =?utf-8?B?VktkakZUSmg1Yk5PU0F2Vnl4Uys4Y1M0eWNQT203SFQ3cE9mNEpMdmJCVjJP?=
 =?utf-8?B?YXo4bDlWSnRnU1BiVlFkOHNCQUNyNi9pOHFCWUQreU1EbWNhRnhrVm1pN29s?=
 =?utf-8?B?MWR2c3c4QnBiV2UydWhmdm0yTkFKNEMyMnk1dzl0aUZNSHk3Nmlob29XaUJK?=
 =?utf-8?B?ZElJZU8vSUYxOGh6QXF0RmVYVU5xVFl2eWhGenNIQkl3K203OWxzaExUQUdE?=
 =?utf-8?B?ejMwTVVYTTlNdlBtM0ZINzBxY0hWUUJaY1MyakZyMmUrb2lnSUZhUXRDVG93?=
 =?utf-8?B?WXpzMkUyV2pIcFJ3ay9uTEJLYWtVbTYvNzdxdHRhVXkxdzl0QVdsVW50dHNY?=
 =?utf-8?Q?uqee7Vw81WxaHXZ60FrND/YnVcy7iX5TRO9bhjm?=
X-OriginatorOrg: windriver.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a64656c3-aa1b-4484-cb87-08d982eda9d2
X-MS-Exchange-CrossTenant-AuthSource: DM8PR11MB5734.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2021 02:05:52.0746
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ddb2873-a1ad-4a18-ae4e-4644631433be
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rDUrmbbZsQucP152rR8svXoWEQySM8kntAcGPZfcSiJ0cDm1YG4+johykrITeQI7tdvLsXtVk7vr/kJJQsIMmQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB3866
X-Proofpoint-ORIG-GUID: f67eDul3iP8-6ogSEWo5LGfonStegE9Z
X-Proofpoint-GUID: f67eDul3iP8-6ogSEWo5LGfonStegE9Z
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.391,FMLib:17.0.607.475
 definitions=2021-09-28_11,2021-09-28_01,2020-04-07_01
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0
 priorityscore=1501 adultscore=0 clxscore=1011 malwarescore=0 bulkscore=0
 lowpriorityscore=0 mlxscore=0 impostorscore=0 suspectscore=0 spamscore=0
 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109230001 definitions=main-2109290010
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/28/21 11:48 PM, Dongliang Mu wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
> 
> On Tue, Sep 28, 2021 at 11:41 PM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
>>
>> On Tue, Sep 28, 2021 at 04:52:07PM +0300, Dan Carpenter wrote:
>>> On Tue, Sep 28, 2021 at 01:58:00PM +0100, Russell King (Oracle) wrote:
>>>>
>>>> This thread seems to be getting out of hand.
>>>
>>> The thread was closed.  We need to revert Yanfei's patch and apply
>>> Pavel's patch.  He's going to resend.
>>>

Sorry for my patch, it is my bad. :( And thanks for Pavel's v2 which 
help to revert mine.

Regards,
Yanfei

>>>> So, I would suggest a simple fix is to set bus->state to
>>>> MDIOBUS_UNREGISTERED immediately _after_ the successful
>>>> device_register().
>>>
>>> Not after.  It has to be set to MDIOBUS_UNREGISTERED if device_register()
>>> fails, otherwise there will still be a leak.
>>
>> Ah yes, you are correct - the device name may not be freed. Also...
>>
>>   * NOTE: _Never_ directly free @dev after calling this function, even
>>   * if it returned an error! Always use put_device() to give up your
>>   * reference instead.
>>
>> So yes, we must set to MDIOBUS_UNREGISTERED even if device_register()
>> fails.
>>
> 
> So we have reached an agreement. Pavel's patch fixes the syzbot link
> [1], other than Yanfei's patch. However, Yanfei's patch also fixes
> another memory link nearby.
> 
> Right?
> 
> [1] https://syzkaller.appspot.com/bug?id=fa99459691911a0369622248e0f4e3285fcedd97
> 
>> --
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!