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] target: Fix a double put in transport_free_session
To:     Lv Yunlong <lyl2019@mail.ustc.edu.cn>, martin.petersen@oracle.com
Cc:     linux-scsi@vger.kernel.org, target-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20210323025851.11782-1-lyl2019@mail.ustc.edu.cn>
From:   michael.christie@oracle.com
Message-ID: <9d02b016-c924-79e3-9593-c073da0c769d@oracle.com>
Date:   Tue, 23 Mar 2021 11:28:35 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.8.1
In-Reply-To: <20210323025851.11782-1-lyl2019@mail.ustc.edu.cn>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [20.15.0.5] (73.88.28.6) by DM5PR18CA0064.namprd18.prod.outlook.com (2603:10b6:3:22::26) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3955.18 via Frontend Transport; Tue, 23 Mar 2021 16:28:37 +0000
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: a5ce6aa2-e7fa-4d83-fc05-08d8ee18b626
X-MS-TrafficTypeDiagnostic: BY5PR10MB4226:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BY5PR10MB4226C84C6068D80B60D56317F1649@BY5PR10MB4226.namprd10.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:5797;
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: VoBW5mp93IghqmQ2Lbac2Xf9lZ37kWhRmHlfyCpIK7NOloKcJy0MyjJkhCQ7qaTD7la/0ZZR5dU44YuM8OdshyIvA6UgBWigkDCjih8nz4OJI5Yi56JCNj8A854ZtgLA18pEekT4ahyHaY3tt20egLjDi7uqOJNSbIIGTboLzNuumu9Un0yBMip3L+WuiIqxmm6XZ2jj8/rGtCvHm1VWURU5x1zdgLrWlcuVxA0uKW5DwPy6/A/UwCj3h30YSjZS40RyQ33m6NcWiiMq0gxdewl/I3c2mJFVZehQhulwFHf+LqgDqyRm5+eklLZQxUxmwoIZSs1hKN/ZQ5koDuvp0T4EIPeSCub6gmSys7IcvRTV5Yu25GrhD2U2ybwvkpUqWdN+xYlojOObyzupERjmqRER4X/fCIpd9vewMuBXvP0CYOllcKJeogrJXr/LALjMIvvPLR1QdqlyzviU8zbMlHxTe8oOf61cLEuTW3nsxxtOobZMpccv6vd1cYR+ibiPpb7e5yJOGnM1XgijNDapEog1k2/AptFmJYEJcBD024MmY/tLitHQyC7eaUuKZMXxwNBoJzz3EFinBBY0Nv0mbtBUG5YUKbw+sVDs05C1YyzdTpraPrBZykraq77IYvfDhn17W0j5FNLCYCvCeR0NzuB2HlEmm9JtG2OjdPEVwxA+dQfjYyD+7i4sbn45Use/pSWWwzcJqacHMrkqtw0fHQ==
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BYAPR10MB3573.namprd10.prod.outlook.com;PTR:;CAT:NONE;SFS:(366004)(376002)(39860400002)(346002)(136003)(396003)(9686003)(26005)(2906002)(478600001)(66946007)(16576012)(316002)(5660300002)(86362001)(8676002)(66476007)(4326008)(31686004)(66556008)(6486002)(186003)(16526019)(8936002)(53546011)(38100700001)(2616005)(83380400001)(31696002)(6706004)(36756003)(956004)(78286007)(43740500002);DIR:OUT;SFP:1101;
X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?NHhlbExQdDBJOXJkNHFZNXliNVJrYmlUWFhYSlhScFZrOTYwU2ZLZXA5VWVv?=
 =?utf-8?B?aFUxZGdma3VtcW95OU5oTm5ROXpMUm1DRHBOQXlCRXpuVncxcmhGU2JwbHg3?=
 =?utf-8?B?THJwTTR6cTVWUlY5b3QxMkxHZmJxeVBXUDdqRUhTQXhUZVJUdzIvVFNENWJz?=
 =?utf-8?B?MythUHlQRnZkakk1c1A0a21YWU9aN050MS9VTGlaNUcySDhwbDJhVUZMRWRK?=
 =?utf-8?B?aENPQlZxUVo5MXRDSkJ4eXdGb2JEa0NKQWhEc0FwdCtUUmdBRkpzang1THdB?=
 =?utf-8?B?eVowL1d4L3lES0NDZHJlLys2K25mTGgrT3RIL1lxcWJ0MTZneDluN0ozNDZ6?=
 =?utf-8?B?bkpLYUlzYVJIYk5CMWw1WjhJTFNSazNObVNRQWtkWWk0cW1UcWJFY3QraTMv?=
 =?utf-8?B?cE9VQmxZUU1pT2ZoRVhMSUZKU2p1cmdCV1pQVDVjRU8wN1VFOENwMG1Xckx1?=
 =?utf-8?B?V0hobkp6M1FDV21wMS9wcU1qWXYrczZFSUc3WVZMTWNtRjRuODg0OWh3ZDh1?=
 =?utf-8?B?d3NGTGhhZ1F4dFR6L2c5WGlzcnZPOGpibDQ2Rm5kQlRnUW1odFlQb3Z2K1Bs?=
 =?utf-8?B?eTQrWnFXQ2dRN01lVmZMajlXRnZvdHE4RFFJcEFHVE1XL1IrK2k1ZEU5NjEz?=
 =?utf-8?B?QklWbTE3VVN4R2wvRnF5TWtTS0ZyNitIcThFSFA3RDRoOHlxdktCTFZlZ1pr?=
 =?utf-8?B?WkZoL3pUR0l5MEVLSWVjMDVDQjlxanNWM2NZYU9yRkJMNjhrYlBiRDVjRFUw?=
 =?utf-8?B?NS93R0tGNVFEbVVCeWZzZFc1T2pScTRwNEVNRDFNT29oczBrL1ljYmRDMWJy?=
 =?utf-8?B?ay9LVWN3YzZNWUJBUHBXaXlncW9oSGF2VHoxUWFXc0ExK2QwdHFxYzZ0RTlR?=
 =?utf-8?B?bHlzRTUwODlpcFd6NDNhZ01JMXRwektOQUZkYnhqc0xTL0RvbForNEFCMU5l?=
 =?utf-8?B?end1SDRRQnNvQklBckVhM2ZKT0hTQlZoZHVGOUZjb3Q3UGJ0aE5ldlQvWUIy?=
 =?utf-8?B?WnUwRURBMGFiMEhlT253TEU0bVVZNW1SbE5vWE1FUTlkYWdzcDVCdVRIU0ZJ?=
 =?utf-8?B?WERidVJEMlkzMWNNZGdVUHg4OCtBK1Y1S0RiY01HM0QydjZlZlhCdm5HMUJa?=
 =?utf-8?B?U3QyOU54QW1ucXlIWmtTSGZpdWtoOHZ0bkZTQXQ2RGY5dS9SK1hGSkIrVGtL?=
 =?utf-8?B?bk0wbkV5SXphYmcvYVc3MzRPTE9tbHdQYjhmajdiTE9YSnkwTVp1KzA0TCtl?=
 =?utf-8?B?K1NGTkpKbUR3dmM2WVVETXZnY3U1QTVaVFFHaVM1KzkydHVWUEVJWFgvNHZ3?=
 =?utf-8?B?U3dSTFU1b0pCcEZxc1JzMWFQTnROZ0crRk0vSjA1KzJpN3IyZzU0aVhyZnQ5?=
 =?utf-8?B?LytzZW1ScGVySU5sNWJZQ0NNNUoyN1AwL0ZwN1kwQXVJMWlLOFQ0WTB3ZGt6?=
 =?utf-8?B?NWd5ejJFQVVPNlpEMmR4MUcwOTdPZmp0RGtJeGRoMnIwWTdlYVRPWjgzK29V?=
 =?utf-8?B?SGQvNDlZZG4zckl2YkVzVEV5K01sYk5LL2VPODdyTm9zbDRDd0wxMFpOZVl4?=
 =?utf-8?B?VWY1ZDhVVXc0cWF3SW5sUXdPUVYxYkR2RjNQcjNDYzRXdTNJSnZwb01SRzkx?=
 =?utf-8?B?VGowdTJjWjY2ZUZLa1BYUDNTL0krRHhvaFNNVC9oYXhrMnRFTk4yUEZNUElK?=
 =?utf-8?B?STlldmcvemhtdjVaVElaQzMxUXdtZ29oNm54UkREaHVEUGVnUjVuL29Rdzhj?=
 =?utf-8?Q?OFMSXPsOwjhci1/jEEuHauTOxrts/YDVNQtVtmL?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a5ce6aa2-e7fa-4d83-fc05-08d8ee18b626
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB3573.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Mar 2021 16:28:37.9686
 (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: N1QGXEBv1i1byYehc+hKAjwO01Zg2UAKyYrDbJBHNqFXn1u2w1sVhpOy+ORXC+Ml7x+AL+M+06GNcm4G1CiEvgt1ZTB5pSk2NBZ+E4MV5ms=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4226
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9932 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxlogscore=999
 malwarescore=0 phishscore=0 bulkscore=0 mlxscore=0 suspectscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2103230119
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9932 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 lowpriorityscore=0 phishscore=0
 mlxlogscore=999 priorityscore=1501 impostorscore=0 bulkscore=0 spamscore=0
 adultscore=0 clxscore=1011 malwarescore=0 mlxscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2103230119
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/22/21 9:58 PM, Lv Yunlong wrote:
> In transport_free_session, se_nacl is got from se_sess
> with the initial reference. If se_nacl->acl_sess_list is
> empty, se_nacl->dynamic_stop is set to true. Then the first
> target_put_nacl(se_nacl) will drop the initial reference
> and free se_nacl. Later there is a second target_put_nacl()
> to put se_nacl. It may cause error in race.
>> My patch sets se_nacl->dynamic_stop to false to avoid the
> double put.
> 
> Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
> ---
>  drivers/target/target_core_transport.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 5ecb9f18a53d..c266defe694f 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -584,8 +584,10 @@ void transport_free_session(struct se_session *se_sess)
>  		}
>  		mutex_unlock(&se_tpg->acl_node_mutex);
>  
> -		if (se_nacl->dynamic_stop)
> +		if (se_nacl->dynamic_stop) {
>  			target_put_nacl(se_nacl);
> +			se_nacl->dynamic_stop = false;
> +		}
>  
>  		target_put_nacl(se_nacl);
Could you describe the race a little more?

Is the race:

1. thread1 called core_tpg_check_initiator_node_acl and found the acl.
sess->se_node_acl is set to the found acl.
2. thread2 is running transport_free_session. It now grabs the acl_node_mutex
and sees se_nacl->acl_sess_list is empty.
3. thread2 does the dynamic_stop=true operations in transport_free_session.
4. thread1 now calls transport_register_session now adds the sess to acl's
acl_sess_list.

Later when the session that thread 1 created is deleted dynamic_stop is still
set, so we do an extra target_put_nacl?

I'm not sure your patch will handle this race. When we delete the session thread1
created dynamic_node_acl is still set, so this:

                mutex_lock(&se_tpg->acl_node_mutex);
                if (se_nacl->dynamic_node_acl &&
                    !se_tfo->tpg_check_demo_mode_cache(se_tpg)) {
                        spin_lock_irqsave(&se_nacl->nacl_sess_lock, flags);
                        if (list_empty(&se_nacl->acl_sess_list))
                                se_nacl->dynamic_stop = true;

can set dynamic_stop to true again and we can end up doing the extra put still.

On top of the extra put we also do

list_del(&se_nacl->acl_list);

twice so we have to handle that as well.

Is there also another bug in this code. If someone adds an acl while there is a
dynamic acl in place core_tpg_add_initiator_node_acl will clear dynamic_node_acl
but we leave the extra reference, so later when transport_free_session is called
we will not do the extra put.