From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E9051C433EF for ; Mon, 14 Mar 2022 23:45:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343755AbiCNXqz (ORCPT ); Mon, 14 Mar 2022 19:46:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50346 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239436AbiCNXqx (ORCPT ); Mon, 14 Mar 2022 19:46:53 -0400 Received: from mail-ej1-x633.google.com (mail-ej1-x633.google.com [IPv6:2a00:1450:4864:20::633]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7072B3E5F1 for ; Mon, 14 Mar 2022 16:45:41 -0700 (PDT) Received: by mail-ej1-x633.google.com with SMTP id hw13so36166508ejc.9 for ; Mon, 14 Mar 2022 16:45:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=mMcKlcLkl25csYGtmWB99GmowZ54MQeoPUbWxxcyFU0=; b=bp+YoIVGfFN/43D3vqHErY1NhIRJApUAhtw6o/hQeIyaDBqIfE66EIhFuNlzouKqsM ICMTNUqtiRAq88ITvYxr1uiF5OmdPGah32szvxsrVE8+TbWCoOGxOG6L25aPWaho7kdy 3FU/jNh6QDkaRKObZiMMOkBlD8Aqv99swKSc8YZ9bFy6T0F/mbML6sVVYqbR6qCMVE6H bYhE/ZUbxEYhn6H6a+/VharXFDIBFPo45r3fdwvvEZ51SgbTUi25vilSjvica56rkM6I xTjLcOE3UXJzEYpLAj8t0ISFMLBTpHiYS87O0Yj4+2ZJb/rKhYVFzvJMGee+en585/rw GBaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=mMcKlcLkl25csYGtmWB99GmowZ54MQeoPUbWxxcyFU0=; b=2DYJaiznlr468lVJi0UP99ryW0CRBAgcJ8p0/+FKAmgDiD4cD1XQLrC4m+LKn4fbsy dBq2Env84d4rXMaSzaXaRxQ3CwLirK4g6oWGo1JKbRG/SCP94Y7U495tDR925uvxYuWi U0hMc54sV+mNCC2uPwzFMeGrWKFED1N7v6IicTmqOhSTgr2iQV54i3eNX07sgC9ZlTiJ kSvMedkbzp7lSkWVjSqHg61zDYHRlFs3pNwTh57KpQfRAIwDNSVjiT1XPr5XGoTTRkDB DBQqMSKlz0f181ee3k9i8wCZMSGzjUefsHLPb54++vW7z29bO1XDcFOOZf5stEMF9dBu yAng== X-Gm-Message-State: AOAM532UUNUmIaWg/VCoFC9JqddWi+AKrJ1JumR4eGPr80Tgm4EnsD0v IU6t2kbaYqyPPbbI0x7TyQLiAOhbLNEUv1xt7tq2uw== X-Google-Smtp-Source: ABdhPJzivdKSx3a2ptS01jjlhP74llQ2PdFx6OA83cn+BAtPtg6OtNsv++AhnKe1mUMAW8amvG7lkgIAO9oTvOh6yGc= X-Received: by 2002:a17:906:5d08:b0:6da:b4ea:937 with SMTP id g8-20020a1709065d0800b006dab4ea0937mr20516098ejt.446.1647301539672; Mon, 14 Mar 2022 16:45:39 -0700 (PDT) MIME-Version: 1.0 References: <20220309165222.2843651-1-tjmercier@google.com> <20220309165222.2843651-8-tjmercier@google.com> In-Reply-To: From: "T.J. Mercier" Date: Mon, 14 Mar 2022 16:45:28 -0700 Message-ID: Subject: Re: [RFC v3 7/8] binder: use __kernel_pid_t and __kernel_uid_t for userspace To: Todd Kjos Cc: Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Jonathan Corbet , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Todd Kjos , Martijn Coenen , Joel Fernandes , Christian Brauner , Hridya Valsaraju , Suren Baghdasaryan , Sumit Semwal , =?UTF-8?Q?Christian_K=C3=B6nig?= , Benjamin Gaignard , Liam Mark , Laura Abbott , Brian Starkey , John Stultz , Tejun Heo , Zefan Li , Johannes Weiner , Shuah Khan , Kalesh Singh , Kenny.Ho@amd.com, dri-devel@lists.freedesktop.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org, cgroups@vger.kernel.org, linux-kselftest@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 10, 2022 at 11:33 AM Todd Kjos wrote: > > On Wed, Mar 9, 2022 at 8:52 AM T.J. Mercier wrote: > > > > The kernel interface should use types that the kernel defines instead o= f > > pid_t and uid_t, whose definiton is owned by libc. This fixes the heade= r > > so that it can be included without first including sys/types.h. > > > > Signed-off-by: T.J. Mercier > > --- > > include/uapi/linux/android/binder.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/a= ndroid/binder.h > > index 169fd5069a1a..aa28454dbca3 100644 > > --- a/include/uapi/linux/android/binder.h > > +++ b/include/uapi/linux/android/binder.h > > @@ -289,8 +289,8 @@ struct binder_transaction_data { > > > > /* General information about the transaction. */ > > __u32 flags; > > - pid_t sender_pid; > > - uid_t sender_euid; > > + __kernel_pid_t sender_pid; > > + __kernel_uid_t sender_euid; > > Are we guaranteed that this does not affect the UAPI at all? Userspace > code using this definition will have to run with kernels using the old > definition and visa-versa. A standards compliant userspace should be expecting a signed integer type here. So the only way I can think userspace would be affected is if: 1) pid_t is a long AND 2) sizeof(long) > sizeof(int) AND 3) Consumers of the pid_t definition actually attempt to mutate the result to make use of extra bits in the variable (which are not there) This seems extremely unlikely. For instance just on the topic of the first item, all of the C library implementations with pid_t definitions linked here use an int, except for Bionic which typdefs pid_t to __kernel_pid_t and Sortix which uses long. https://wiki.osdev.org/C_Library However I would argue this is already broken and should count as a bug fix since I can't do this: $ cat binder_include.c ; gcc binder_include.c #include int main() {} In file included from binder_include.c:1: /usr/include/linux/android/binder.h:291:9: error: unknown type name =E2=80= =98pid_t=E2=80=99 291 | pid_t sender_pid; | ^~~~~ /usr/include/linux/android/binder.h:292:9: error: unknown type name =E2=80= =98uid_t=E2=80=99 292 | uid_t sender_euid; | ^~~~~ This is also the only occurrence of pid_t in all of include/uapi/linux. All 40+ other uses are __kernel_pid_t, and I don't see why the binder header should be different. > > > binder_size_t data_size; /* number of bytes of data */ > > binder_size_t offsets_size; /* number of bytes of offsets *= / > > > > -- > > 2.35.1.616.g0bdcbb4464-goog > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 997A3C433EF for ; Mon, 14 Mar 2022 23:45:42 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id F019E10E311; Mon, 14 Mar 2022 23:45:41 +0000 (UTC) Received: from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com [IPv6:2a00:1450:4864:20::62a]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7E57010E311 for ; Mon, 14 Mar 2022 23:45:41 +0000 (UTC) Received: by mail-ej1-x62a.google.com with SMTP id qx21so37528130ejb.13 for ; Mon, 14 Mar 2022 16:45:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=mMcKlcLkl25csYGtmWB99GmowZ54MQeoPUbWxxcyFU0=; b=bp+YoIVGfFN/43D3vqHErY1NhIRJApUAhtw6o/hQeIyaDBqIfE66EIhFuNlzouKqsM ICMTNUqtiRAq88ITvYxr1uiF5OmdPGah32szvxsrVE8+TbWCoOGxOG6L25aPWaho7kdy 3FU/jNh6QDkaRKObZiMMOkBlD8Aqv99swKSc8YZ9bFy6T0F/mbML6sVVYqbR6qCMVE6H bYhE/ZUbxEYhn6H6a+/VharXFDIBFPo45r3fdwvvEZ51SgbTUi25vilSjvica56rkM6I xTjLcOE3UXJzEYpLAj8t0ISFMLBTpHiYS87O0Yj4+2ZJb/rKhYVFzvJMGee+en585/rw GBaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=mMcKlcLkl25csYGtmWB99GmowZ54MQeoPUbWxxcyFU0=; b=39GyRPGJ1bgOwIQ9I1KjNA4RnavR8EjZ94Z6ocgYvWKx0eKU65xixcAZbomtodmt7S b7fRnLwCnDffaxmqTYXzRUQWQ+V2g39oXdiyG/EVQ/mx8TVlExwSOQL14Ksr5DoS3/5h HsD5wFbFc+eONAKpBmtsv54az3fMD2NifKDoPmPX17hwDmoGo4xo+NMVirU4VglOkME1 Jgd2+fHABo3oPNAVsmQg79Bxar5sBKTKwZofnY+u68bQPQ4f3xM2hEHEj0KaddlNpvNp K46Pq7Db/bbC7BtSOJQ724CxRr/qBBsASg5tp8Eq5xFLCs9lyXGyzTgrwgty+SsORNd8 ONww== X-Gm-Message-State: AOAM531nIZ2J1GjUm0fWa2ccqvRYDmzZ/aKONYf9vYl2zk2ejoR/kMYM xOeqvcNOJ9rmLY3nkDMGbus39pbbZpLx2GyYIKc/fA== X-Google-Smtp-Source: ABdhPJzivdKSx3a2ptS01jjlhP74llQ2PdFx6OA83cn+BAtPtg6OtNsv++AhnKe1mUMAW8amvG7lkgIAO9oTvOh6yGc= X-Received: by 2002:a17:906:5d08:b0:6da:b4ea:937 with SMTP id g8-20020a1709065d0800b006dab4ea0937mr20516098ejt.446.1647301539672; Mon, 14 Mar 2022 16:45:39 -0700 (PDT) MIME-Version: 1.0 References: <20220309165222.2843651-1-tjmercier@google.com> <20220309165222.2843651-8-tjmercier@google.com> In-Reply-To: From: "T.J. Mercier" Date: Mon, 14 Mar 2022 16:45:28 -0700 Message-ID: Subject: Re: [RFC v3 7/8] binder: use __kernel_pid_t and __kernel_uid_t for userspace To: Todd Kjos Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Zefan Li , linux-doc@vger.kernel.org, David Airlie , dri-devel@lists.freedesktop.org, Benjamin Gaignard , Kalesh Singh , Joel Fernandes , Shuah Khan , Sumit Semwal , Kenny.Ho@amd.com, Jonathan Corbet , Martijn Coenen , Laura Abbott , linux-media@vger.kernel.org, linux-kselftest@vger.kernel.org, Todd Kjos , linaro-mm-sig@lists.linaro.org, Tejun Heo , cgroups@vger.kernel.org, Suren Baghdasaryan , Christian Brauner , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Liam Mark , =?UTF-8?Q?Christian_K=C3=B6nig?= , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Thomas Zimmermann , Johannes Weiner , Hridya Valsaraju Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Thu, Mar 10, 2022 at 11:33 AM Todd Kjos wrote: > > On Wed, Mar 9, 2022 at 8:52 AM T.J. Mercier wrote: > > > > The kernel interface should use types that the kernel defines instead o= f > > pid_t and uid_t, whose definiton is owned by libc. This fixes the heade= r > > so that it can be included without first including sys/types.h. > > > > Signed-off-by: T.J. Mercier > > --- > > include/uapi/linux/android/binder.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/a= ndroid/binder.h > > index 169fd5069a1a..aa28454dbca3 100644 > > --- a/include/uapi/linux/android/binder.h > > +++ b/include/uapi/linux/android/binder.h > > @@ -289,8 +289,8 @@ struct binder_transaction_data { > > > > /* General information about the transaction. */ > > __u32 flags; > > - pid_t sender_pid; > > - uid_t sender_euid; > > + __kernel_pid_t sender_pid; > > + __kernel_uid_t sender_euid; > > Are we guaranteed that this does not affect the UAPI at all? Userspace > code using this definition will have to run with kernels using the old > definition and visa-versa. A standards compliant userspace should be expecting a signed integer type here. So the only way I can think userspace would be affected is if: 1) pid_t is a long AND 2) sizeof(long) > sizeof(int) AND 3) Consumers of the pid_t definition actually attempt to mutate the result to make use of extra bits in the variable (which are not there) This seems extremely unlikely. For instance just on the topic of the first item, all of the C library implementations with pid_t definitions linked here use an int, except for Bionic which typdefs pid_t to __kernel_pid_t and Sortix which uses long. https://wiki.osdev.org/C_Library However I would argue this is already broken and should count as a bug fix since I can't do this: $ cat binder_include.c ; gcc binder_include.c #include int main() {} In file included from binder_include.c:1: /usr/include/linux/android/binder.h:291:9: error: unknown type name =E2=80= =98pid_t=E2=80=99 291 | pid_t sender_pid; | ^~~~~ /usr/include/linux/android/binder.h:292:9: error: unknown type name =E2=80= =98uid_t=E2=80=99 292 | uid_t sender_euid; | ^~~~~ This is also the only occurrence of pid_t in all of include/uapi/linux. All 40+ other uses are __kernel_pid_t, and I don't see why the binder header should be different. > > > binder_size_t data_size; /* number of bytes of data */ > > binder_size_t offsets_size; /* number of bytes of offsets *= / > > > > -- > > 2.35.1.616.g0bdcbb4464-goog > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: "T.J. Mercier" Subject: Re: [RFC v3 7/8] binder: use __kernel_pid_t and __kernel_uid_t for userspace Date: Mon, 14 Mar 2022 16:45:28 -0700 Message-ID: References: <20220309165222.2843651-1-tjmercier@google.com> <20220309165222.2843651-8-tjmercier@google.com> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=mMcKlcLkl25csYGtmWB99GmowZ54MQeoPUbWxxcyFU0=; b=bp+YoIVGfFN/43D3vqHErY1NhIRJApUAhtw6o/hQeIyaDBqIfE66EIhFuNlzouKqsM ICMTNUqtiRAq88ITvYxr1uiF5OmdPGah32szvxsrVE8+TbWCoOGxOG6L25aPWaho7kdy 3FU/jNh6QDkaRKObZiMMOkBlD8Aqv99swKSc8YZ9bFy6T0F/mbML6sVVYqbR6qCMVE6H bYhE/ZUbxEYhn6H6a+/VharXFDIBFPo45r3fdwvvEZ51SgbTUi25vilSjvica56rkM6I xTjLcOE3UXJzEYpLAj8t0ISFMLBTpHiYS87O0Yj4+2ZJb/rKhYVFzvJMGee+en585/rw GBaA== In-Reply-To: List-ID: Content-Type: text/plain; charset="utf-8" To: Todd Kjos Cc: Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Jonathan Corbet , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Todd Kjos , Martijn Coenen , Joel Fernandes , Christian Brauner , Hridya Valsaraju , Suren Baghdasaryan , Sumit Semwal , =?UTF-8?Q?Christian_K=C3=B6nig?= , Benjamin Gaignard , Liam Mark , Laur On Thu, Mar 10, 2022 at 11:33 AM Todd Kjos wrote: > > On Wed, Mar 9, 2022 at 8:52 AM T.J. Mercier wrote: > > > > The kernel interface should use types that the kernel defines instead o= f > > pid_t and uid_t, whose definiton is owned by libc. This fixes the heade= r > > so that it can be included without first including sys/types.h. > > > > Signed-off-by: T.J. Mercier > > --- > > include/uapi/linux/android/binder.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/linux/android/binder.h b/include/uapi/linux/a= ndroid/binder.h > > index 169fd5069a1a..aa28454dbca3 100644 > > --- a/include/uapi/linux/android/binder.h > > +++ b/include/uapi/linux/android/binder.h > > @@ -289,8 +289,8 @@ struct binder_transaction_data { > > > > /* General information about the transaction. */ > > __u32 flags; > > - pid_t sender_pid; > > - uid_t sender_euid; > > + __kernel_pid_t sender_pid; > > + __kernel_uid_t sender_euid; > > Are we guaranteed that this does not affect the UAPI at all? Userspace > code using this definition will have to run with kernels using the old > definition and visa-versa. A standards compliant userspace should be expecting a signed integer type here. So the only way I can think userspace would be affected is if: 1) pid_t is a long AND 2) sizeof(long) > sizeof(int) AND 3) Consumers of the pid_t definition actually attempt to mutate the result to make use of extra bits in the variable (which are not there) This seems extremely unlikely. For instance just on the topic of the first item, all of the C library implementations with pid_t definitions linked here use an int, except for Bionic which typdefs pid_t to __kernel_pid_t and Sortix which uses long. https://wiki.osdev.org/C_Library However I would argue this is already broken and should count as a bug fix since I can't do this: $ cat binder_include.c ; gcc binder_include.c #include int main() {} In file included from binder_include.c:1: /usr/include/linux/android/binder.h:291:9: error: unknown type name =E2=80= =98pid_t=E2=80=99 291 | pid_t sender_pid; | ^~~~~ /usr/include/linux/android/binder.h:292:9: error: unknown type name =E2=80= =98uid_t=E2=80=99 292 | uid_t sender_euid; | ^~~~~ This is also the only occurrence of pid_t in all of include/uapi/linux. All 40+ other uses are __kernel_pid_t, and I don't see why the binder header should be different. > > > binder_size_t data_size; /* number of bytes of data */ > > binder_size_t offsets_size; /* number of bytes of offsets *= / > > > > -- > > 2.35.1.616.g0bdcbb4464-goog > >