Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e18b49ee-44a3-eb72-ec58-9579dbdf4012@oracle.com>
Date:   Wed, 2 Feb 2022 15:02:12 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.1
From:   Mike Christie <michael.christie@oracle.com>
Subject: Re: [PATCH V6 01/10] Use copy_process in vhost layer
To:     "Eric W. Biederman" <ebiederm@xmission.com>
Cc:     geert@linux-m68k.org, vverma@digitalocean.com, hdanton@sina.com,
        hch@infradead.org, stefanha@redhat.com, jasowang@redhat.com,
        mst@redhat.com, sgarzare@redhat.com,
        virtualization@lists.linux-foundation.org,
        christian.brauner@ubuntu.com, axboe@kernel.dk,
        linux-kernel@vger.kernel.org
References: <20211129194707.5863-1-michael.christie@oracle.com>
 <87tuf79gni.fsf@email.froward.int.ebiederm.org>
 <a171238e-d731-1c22-af72-0f7faf7f4bea@oracle.com>
 <87tuf11oe6.fsf@email.froward.int.ebiederm.org>
 <a3bd6fe7-1775-6fdd-3a02-e779c4d4e498@oracle.com>
 <87pmpoxzuf.fsf@email.froward.int.ebiederm.org>
 <783145b7-243b-b85e-e274-44ef6c0696b9@oracle.com>
 <874k62b76d.fsf@email.froward.int.ebiederm.org>
 <68ba89ae-108e-c14a-02a0-db72b169c9b1@oracle.com>
 <87a6fs3lk5.fsf@email.froward.int.ebiederm.org>
Content-Language: en-US
In-Reply-To: <87a6fs3lk5.fsf@email.froward.int.ebiederm.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Zk5qT3JoNjdwbFdUeUpzYktTWExxdXBiZkpXUXV6WEY1dUlpYlUrcmcwbnpW?=
 =?utf-8?B?SU85NG1aOVYzcFVtcS9GeDNvVEYrYWFQTUJCdW0yNEUzODFSbjQvQ1I2b1I1?=
 =?utf-8?B?NXFjOUtyek5QN0oydUx1TGMvK2VZL1JCMUVHbkt6ZTV3QWVERGI4clVJdHA5?=
 =?utf-8?B?Qm1LQTJEWHAwcUNEWmVmK3gxSEhyOEpWejRWbWhYTzJQbnBsY01YSGxWNWpJ?=
 =?utf-8?B?UHNNQTYrQkEyczJZTWZ5R0RLeWtLZ0owbHp5UVNnNFBReG1EbnVlWnZnTXNm?=
 =?utf-8?B?M1ZlbXhzM3pYeTdZN2E5bWsrcnlMSjhUbXJtR1Zja1FKVGI3VFRwZmlyV2lr?=
 =?utf-8?B?OG01Q01HZWo5ZjBQamM0VG5SSFBtRk5LVHE1bDcwaytsZWhZRkJJUERqUzZ1?=
 =?utf-8?B?VDBsN1JIU0cxVDNlUExEbW9WODgwSUpsQXhOZ1Y3Q1NGMzUrYllHUDhmZ1hY?=
 =?utf-8?B?TEZuWmhmNGFpWGt2R3lRVnVjMzlFWERtZHFrVEllbm9qampDcGRjcThaYlNp?=
 =?utf-8?B?SWdUVjZjd3h2eEdCOWV4MWlUSS9YZzVoSGtZOVRtY2NYdWtpWVZNVkhUNXdh?=
 =?utf-8?B?RWNNZDZmTVdqM0VXWGRVdW5yQ1VEbGU5b29vczlORlc1bWNQek5WWU9KN2RV?=
 =?utf-8?B?QlVSckRaOUlXZjVpaklxVGxwdCtYQnpnWWZiZWFkcU85bmRMbjJiUmVteS80?=
 =?utf-8?B?eU82b05QQ1lId2l1K2lrZEk4Tjljc3Z6N1VESitYaGNFbUpmbjBFRG9OUjg3?=
 =?utf-8?B?L0k0YnQwYXV4UW9hN0VDaGJiU1EvWmZtaUZES21mdktCK0IwTmxWakN1c3Zz?=
 =?utf-8?B?UURMNS9qNXZUVUtnY21MZTNHSzR1dmMrY1VNM1hZOGVQUk84NmJSMjExbGdt?=
 =?utf-8?B?Z3h4YzFmaGd0Vy9ieElxbnZYME5PdEVBaXNEd2JXaGxsYVJoUWY0MDdDdytS?=
 =?utf-8?B?WmxoSUgrdFpLTVd0ZWFZKzV6Tm1UZnNSNEl4bHAzaGdMdkp1ZzVXYVN5Q0NI?=
 =?utf-8?B?dHQ3dkhVNy92M3JzM1ErblhRTjhreVhBRzJTeGpTNVA4YTZuQm9ZVUdja2I3?=
 =?utf-8?B?RUpTcGY0VmJ0WnJKS01uRjZXbkNad3pqbjh6dDNmdW5zZ2c0VGU5WGhiQ09X?=
 =?utf-8?B?QmFFSUdHYjhZVHQ2MTVYakpsQjU2WmNMYzJoeDI3NExKZllRT21hTXRYeWYy?=
 =?utf-8?B?ZTQ3S1NZanllTmRQRm8wRnN5Q3pUZG10Sm11aFlpZW13djFmR3ovSS8vTVAx?=
 =?utf-8?B?bDY2LzN0QUIySWgzOVBLOS9yTXZEa0lNdW5MMmZtRHZkRVBvZXUrNTV2VldY?=
 =?utf-8?B?TUlGK2FJeUdlSGNFRU1Ja25jQ2tqZ2hnT2xjWlR4bEJ3b21JeWhCZlZXVTZM?=
 =?utf-8?B?MERPR3YxK2JkTTRQTXBWOGlya1ZWMzBvaTdZWXZEN2wzck5rRmRwQytCdDNr?=
 =?utf-8?B?bUZ4VlozbkhkeU1xaWZpMzU4ZVIrcU1IN2lyWHRXMFcwaHNBc1lzZ3dTWlpM?=
 =?utf-8?B?R0xKVjNtY2ZnYWJmL1UreXd3bGQwSXlySmVBcWJRUmhiSC9ZRFpRK3ZRVEpy?=
 =?utf-8?B?REZNdVNJYXFsNStyeGpwbDNjUDUwaTRkOHpwL21EQUtVL1M0aEJCNkR3SGxG?=
 =?utf-8?B?RWhLckVyYjlHQWt3RnE5VlNIY2cxNXh5ZC9IVGFNV1dLaGcyWjhYMFd0ZHBR?=
 =?utf-8?B?bWUxV016SEVXdlI3NEczSTQ1ZTVFY2ZybjJFSUVRaGVnNnJjaDdCZWlUWE5k?=
 =?utf-8?B?TnJiQUJiTmFHZlk5ajVhZ3hwYlRBMzUrem1NZXEvNUpjSmQ0Vm81d1ZsbWth?=
 =?utf-8?B?S0RwY3l2N0djK3QyQ3FaWWh2NDhmYzR4WFBOem45eHdaa2E4TzZuMkdmNUlt?=
 =?utf-8?B?eXJ6bHNNdzNXNTVGVExaRXpYalpYQWFmUmdtRU84b3E0alM4UUdlQXAzeWZv?=
 =?utf-8?B?S2FDMmJObENwSEpVaFU1c3lobDRDdDVQU0ZUYVQvbDFBQk10ejJrcHQrc01t?=
 =?utf-8?B?WkxtOUoySmE2KzF2OG1OM2loTXlZU21iZDYva0t1SStVMmNsT0FqU0RPUHBt?=
 =?utf-8?B?U0RRWXp5RllSUUU1eEdkQWxMM29OTUdsdHg1RDJIRDdHK0lrSlpVbjMycmZh?=
 =?utf-8?B?empNR3lpWHlsTEljS24zUkQ5cjBFdWpqTERnMnNsaUVGMlN3dVVNZkFwcDAr?=
 =?utf-8?B?UTBPWnk0Z2wrYWdibms0QkNtUmFpZTVJU3hDQ2lGcWp1SG1TTnBwUkZDcU4r?=
 =?utf-8?B?ZE90aEttVmNVRHpyZkxpVUtQWVRRPT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 83a1a489-d0dd-4f1a-125e-08d9e68f49dc
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2022 21:02:14.4756
 (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: VWN2Xa2v+DpUaED5Ic8EZieZ48/shaTWihivpR7dtkkbeHjTi4EXWL6JXYMv70gNoiAPiRL7TfJJQqZTs7mh1WnK6wuO2RiDCpDK5YD+czM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR10MB4014
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10246 signatures=673430
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 adultscore=0
 suspectscore=0 malwarescore=0 bulkscore=0 mlxscore=0 spamscore=0
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2202020115
X-Proofpoint-GUID: nROq2uTsuoonXRyxdE6f5u_3Yocyu5Uh
X-Proofpoint-ORIG-GUID: nROq2uTsuoonXRyxdE6f5u_3Yocyu5Uh
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 1/18/22 1:12 PM, Eric W. Biederman wrote:
> Mike Christie <michael.christie@oracle.com> writes:
> 
>> On 1/17/22 11:31 AM, Eric W. Biederman wrote:
>>> Mike Christie <michael.christie@oracle.com> writes:
>>>
>>>> On 12/22/21 12:24 PM, Eric W. Biederman wrote:
>>>>> All I am certain of is that you need to set
>>>>> "args->exit_signal = -1;".  This prevents having to play games with
>>>>> do_notify_parent.
>>>>
>>>> Hi Eric,
>>>>
>>>> I have all your review comments handled except this one. It's looking like it's
>>>> more difficult than just setting the exit_signal=-1, so I wanted to check that
>>>> I understood you.
>>>
>>> [snip problems with exit_signal = -1]
>>>
>>>>
>>>> What do you think?
>>>
>>> I was wrong.  I appear to have confused the thread and the non-thread
>>> cases.
>>>
>>> Perhaps I meant "args->exit_signal = 0".  That looks like
>>> do_notify_parent won't send it, and thread_group_leader continues to do
>>> the right thing.
>>
>> That doesn't work too. exit_notify will call do_notify_parent but 
>> our parent, qemu, does not ignore SIGCHILD so we will not drop
>> down in into this chunk:
>>
>>         psig = tsk->parent->sighand;
>>         spin_lock_irqsave(&psig->siglock, flags);
>>         if (!tsk->ptrace && sig == SIGCHLD &&
>>             (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
>>              (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) {
>>
>> do_notify_parent will return false and so autoreap in exit_notify will
>> be false.
> 
> Bah good point.  We won't send the signal but you won't autoreap either.
> 
> I think we could legitimately change this bit:
> 
> 	/*
> 	 * Send with __send_signal as si_pid and si_uid are in the
> 	 * parent's namespaces.
> 	 */
> 	if (valid_signal(sig) && sig)
> 		__send_signal(sig, &info, tsk->parent, PIDTYPE_TGID, false);
> 
> To add:
> 	else
>         	/* We don't notify the parent so just autoreap */
>         	autoreap = true;
> 

Hey,

This works for me, but I think we might have issues where threads now get
reaped too soon when they are being ptraced.

I think I found a simple solution by just using kernel_wait in the vhost
task code since I want to wait for the thread to exit when I'm removing
a device already. I posted a patchset so you can check it out.