Message ID | 1503423981.28672.31.camel@cavium.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 30113 invoked by alias); 22 Aug 2017 17:46:53 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 29733 invoked by uid 89); 22 Aug 2017 17:46:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=one, Hx-languages-length:1380, H*r:sk:NAM02-B, H*r:104.47.38 X-HELO: NAM02-BL2-obe.outbound.protection.outlook.com Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Steve.Ellcey@cavium.com; Message-ID: <1503423981.28672.31.camel@cavium.com> Subject: [PATCH] aarch64: Fix ipc_perm definition for ILP32 From: Steve Ellcey <sellcey@cavium.com> Reply-To: sellcey@cavium.com To: libc-alpha <libc-alpha@sourceware.org> Date: Tue, 22 Aug 2017 10:46:21 -0700 Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-ClientProxiedBy: MWHPR09CA0035.namprd09.prod.outlook.com (10.173.46.149) To MWHPR07MB3551.namprd07.prod.outlook.com (10.164.192.140) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 63d180d6-03c7-44ee-9846-08d4e985b566 X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(2017030254152)(300000503095)(300135400095)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:MWHPR07MB3551; X-Microsoft-Exchange-Diagnostics: 1; MWHPR07MB3551; 3:kFYEP+5iDtpcQ13UFjfkiXu5wmdk8fyIkK8L+iSZOBcvo+bqecZOCB+FIKSA6QrDIzy0rurCEMAnb609CoCeb6XNK9MULr7vIjZPSh3eDdUfz1Zu+GCAlA4s+86Ca+UtL9BTlARFpXLxmzV0x0GFXBFh6taE8jMS60be73szhdxmZLENn8CiLHM/qkxNSSU1GbQPU2Fl9xnTxQNNb4HPI8vnrxXhlDXunCIRUO2dlyFPDysH2Pubdv/wIVPpuzHz; 25:r8U00iazYiljklHOQaiFnRLJpVEIIBhFGw/3GyEGkcx0Kq6sDT4foHOTmokFjGh10gV3nzM8g690SEXNqLCM6s+xaxHmrRYyWSGVXh3sLKgvO89O0CW3plQtaxkF18xBFUsBzkX5CDLjMc5nnuWkMqk3K4qDM36vgISyqNaadKlKQd23ffevULDeejmK+nWj8AssbSek39Ej8Lg9+I6qrQU/GkOOVAfcdM/xaqr0nb56z25Q57MuDa8tD90GKz1ZwYxmgcgO2P7uUnnP18sqEr5JHQNOiIhWzrq20lT7xOybgfPlpiI4lZFEhqZ9g2ZqhUCgLi4VqCxO3JU0Ugi5cw==; 31:SJCDn5io9QaalbmbJ2MxC03DfObxdxsmy7hHfVJJhGgOgSlfDf1jEb0hTOPiFG0iZ8PMInfjULfFCVP46Z2A6yhBouTtVPKxy/Zd9jOYXoZc3MRALHRwfLD4KLmaLwrl7zWbJOpoSaI6jzvZcMdOAwwnLHNL7HtLLoFZCyCA1AKPuG5YPaJ+OsF8pHakc14tgNsB1SmW4BPRyAPd9/NaOuYUHoAEbeG4HhN2DlDskLE= X-MS-TrafficTypeDiagnostic: MWHPR07MB3551: X-Microsoft-Exchange-Diagnostics: 1; MWHPR07MB3551; 20:5XdQ1R5cpMKMzIsCZJ+y+0hLyI6R47AvctHqItW6MvgfQX9x26dp9Nu3QmnFuPuovBfjFExr94FBzeAW2XF10Qr8XbDzUlrqt72XkOqhe983x4IPDju8vouY3NL1IjqbtGdmfqFNMpXqG3QYGSPRUOPf4hBbDnmjIPHO6Wt9LDKPsQHhcdP9d2BKDFem8fq4cfKaoyjfMBWrk9P2EqnF37KmvRD7On+ijRB9fwmUc0DGNIk3IWbqXYXit3XoSR+7d5mmsULPmI3X7Pft4cQZ4tnPDM8UWjOFnQ3RXOuG76A35QM347cyUyydrOMUIyEZFvVJD8ZBYFqTrML3AyJXkA9lveld99kojy4QlE8q1/lgucIIu4C+hR+vU2H5IkOu/IeJ6Rruye+en3/uyuIS8oErZ1D8kklYtRs0K+lBLjrb6VJpQMoqWeBkoX2KTjib8YUauPUybFY2XfEPuKXP9yfXFaKzwzY9UySetxUQeo9q9fG/ruz5JSJwRoAROiYP; 4:TKtcJe5QT6MNzyWGkRqdgVMiF/ztH6hv35EvOSMK/keh5gNIIy8/UoMBvaq3RjlRbYrFw1U6Uzc7BI8xYvkc3Ta4MNK71TxfXHRjbr8yWNAmvTJr+CFB0Wx0Jp26pLfBQA0UuRQu4+yAngB655Af6qdO0I8yPp28Baj6zlPtQitpvlbt3bULBfxTIB50ybGMmBJKzLeYufki5LproholB1e52uHNvTFYgPY8g0sfyldALmcbhSXa5wBupPzTdydV X-Exchange-Antispam-Report-Test: UriScan:; X-Microsoft-Antispam-PRVS: <MWHPR07MB355175A424E9599B09F2B7C1F5840@MWHPR07MB3551.namprd07.prod.outlook.com> X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(601004)(2401047)(8121501046)(5005006)(10201501046)(100000703101)(100105400095)(93006095)(93001095)(3002001)(6041248)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123562025)(20161123564025)(20161123558100)(20161123555025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:MWHPR07MB3551; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:MWHPR07MB3551; X-Forefront-PRVS: 04073E895A X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(7370300001)(4630300001)(6009001)(377424004)(189002)(199003)(23676002)(81156014)(2870700001)(69596002)(2906002)(43066003)(101416001)(478600001)(72206003)(7350300001)(189998001)(47776003)(66066001)(50986999)(25786009)(103116003)(5660300001)(97736004)(6486002)(68736007)(3450700001)(110136004)(6506006)(8676002)(6512007)(6666003)(33646002)(53416004)(6116002)(50226002)(106356001)(105586002)(50466002)(3846002)(305945005)(42186005)(81166006)(7736002)(5820100001)(6916009)(36756003)(53936002)(99106002); DIR:OUT; SFP:1101; SCL:1; SRVR:MWHPR07MB3551; H:sellcey-dt.caveonetworks.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; Received-SPF: None (protection.outlook.com: cavium.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtNV0hQUjA3TUIzNTUxOzIzOndJa1YwZ2hKYXFaOW51WVhpVEhTWWVHb0Iw?= =?utf-8?B?eU9PMWtCQkY1VDY2TlZSYStVR3pZMWpadW1aaitCa29aUlhwbzZ5SGNHNy96?= =?utf-8?B?WE9WRDdYWXllMTBrd2FvejNUWlA1QkJjRGJWYW5wZ1c4NEJYeVVHMllPZjVE?= =?utf-8?B?c2VhUjNuZG02NkZjdXhWc1NzSjVodmhpcGVPMGUxZmpzTnNkQmxzWk1QVUJX?= =?utf-8?B?TldEN3ZrOTRhZkROZUdGTjI2VG5KMkdDOEJpaVRGWExiY2JPa1NqbjNNa2ha?= =?utf-8?B?R2F3VXhJNDl5QU1YOFRuWnJwbUszR0wzelJwUnpiTE5ZUzY4aWNOYmJ0ZkRz?= =?utf-8?B?S1FvMFozZlB2ejMzYWg3RG45Q0JIYjZ3V21OUzNXa1NuK1FvY0pSVTlKOXdL?= =?utf-8?B?all4QmVNZ3YrSHdkUUFXT0tLQ0NFdVJqc3BtSm5GTjI0dEtYN20vZjMxYVhZ?= =?utf-8?B?c25tL210a3AvMjcrZWFLVVJSRnIxV3hrMVpYMjJoVXdXVFFCYzUxbk45alpm?= =?utf-8?B?R1EwWGZJMk5xQ1hYYUxoMkkrVERhbnVaKzJaN2NmditWMWVMejNMR2lqRElm?= =?utf-8?B?anBuZjROS1c4OUV0bC9xbU1nZ3htRmI4OVJHK2ZJc3BVQkd3OVVqZExhQWti?= =?utf-8?B?WFVXYnlvalFLa3hoMlc4bTFxbzBqR3RZcE44dFJXR2FYZEo5UHRFSGg2MHNV?= =?utf-8?B?eGJhdHoyM1VpSEk1WUxKb2dFMHVseW9hbDVUTTc0YlA4VGxxUGlUZHppbWxx?= =?utf-8?B?bDIrUVlmUVliN2dHUk9Pcm1KQ3BvSm5oNEV0cGF1aVUrKzFpaWJ0NzRZYmxW?= =?utf-8?B?T3gvclJ3SUI0REVDaFprZ1Y1ZHNvUFdOc1EwczV4dXVtN29mTysvOHV6TzJB?= =?utf-8?B?eXhaai93eVRGeXBBcDJhTlcyYzFqcURwc1A1dW0wV01QUlZZWi9NcHJtcUhW?= =?utf-8?B?a3BOYU1POFJIb1hScWVLSlFUVGJIQklOd0VQdjB3OTZwcExoVmtSR1pXRmVz?= =?utf-8?B?czdvSDhBWDc5b29Hd0JzVmhQQ2I4R0hUcFJuZkxnNTAzeXJNYWtTQjdiaURM?= =?utf-8?B?OXZzSjRsekh6NXZ4WjdMdVp1ejZzcGg5KzVGcnlqUUpUUUlVdEI1M2RVd3Ry?= =?utf-8?B?WDl1SGsyNGdWMVZaemRkaUliZzF2WFk1ZWJqZXgvNGNMY2s4MnM4UG1WL0gv?= =?utf-8?B?ZHNWS2trUzJaSkxGSXhya0E5S0JmUm9LU0l3YVA3MnFlNFQ4a1JqMW5wMitC?= =?utf-8?B?MS9wRU8vZ2JuZ3dCM1lya3AzVENBeTlJSlJMeHBuZXdMUHhLeGV6NHQ3STdQ?= =?utf-8?B?K2R2N1owMkFSeWY0c0FDcUFTVHJLVTMvSVpsamF6UUlTY0RPbXo1QVF3N0J1?= =?utf-8?B?T05KOUhhL0xKNEFUT0lZb3M4aHUvd0Z6RWVNT3hETDdjN3dSdSsxam81bjlv?= =?utf-8?B?L0d6MjBrMEZ0TktIdk8za2RacmRQQkZ1WTdESEdINE1yYjRZWGdFcXNZZkl0?= =?utf-8?B?a3ZNbktuYnhaTGcreG14NU4xYmx0UW1nTTJqbm1ZeVhoUEZ2amVFTzZ3YlEy?= =?utf-8?Q?a/+jfsto7xN27cukavY70yqc1kZ7hmVuqC8TNfMKeXgA=3D?= X-Microsoft-Exchange-Diagnostics: 1; MWHPR07MB3551; 6:yp6Gjh8VND3ekmweNcRXkNYMeGsQEhx+sap61xfHHrXaAHkOr102S/1uuWK+tdoHg2ivBlPLiEWMLUnZ02hE9C7UU26TGTkgn5UaZO7YquYkZEfzxz320vP28eQUwwB9wos8rG5E1RbQ6Rd4Jz51JB9LdTo2umfaJIdyFrKh7h6OBBLikYf3A+8THOle9K6oI19t8x8Wb1OZKMUaoDpiTCdn6v5jwgnntadi5BAYXwlTY9TWx1XLqP5QGbe6SzYEsQ0PkoDZg1e4EULDoPq9eC5RfaABIoWlESD0B9blnkAFsSH0SYjSM79CaHcHjcDMyXaq+5cWfNTsVeFwwEgaqw==; 5:CK21lyVlNf/zIMUY6nDGTWGe2EQ8vCaga/KEPlzztS9Gk08ZfDt+8QSaiSgn5L4FLnwAMdHhNhDKAlUCSMjcAfrUh6DZcU20AEpddzGPMiRleb3qIKgHjr4ebXuuTVBz3CFFUU+Lq+V/mGQ03Vl9Gw==; 24:GBmliyuh8dPhVBJAw7rkVGin7uVNIMaiaYDXg88LbZz/8OnMa2zd2LSVczUderVakY8bYdjDyGmO+zpYwgLI4wjlK7AVrpiQYAGXlRaG8lA=; 7:B2yze7X7F49qCrZ2PVk9xIYJN3kJumTWhLPMvj9js4jrdp0UZEP3criOXfq8yUESPcq+KsWcB/a73131AiUtU5bwuc4v0FZMOcLl0MjTp/tx7Z9+sZVPc5LVaFBHbUE5IE72GEcoBNnp4X49q2wTS25sobkaX5KKMnqeOjrevWV9NQljf9mxiURQkgxcza4hlrB1iKqDbFtBtMxfF/TZvOyrCFiuwuUy6xGvzqCjC5Q= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: cavium.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Aug 2017 17:46:24.8310 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR07MB3551 |
Commit Message
Steve Ellcey
Aug. 22, 2017, 5:46 p.m. UTC
Here is another aarch64 ILP32 patch. The mode field in ipc_perm in ILP32 should be a 16 bit field, not a 32 bit one. This was out-of-sync with what the kernel had. This was causing sysvipc/test-sysvsem to fail in ILP32 mode. This change is only needed for ILP32 so it doesn't need to go in until we add that ABI but I am sending out for review and comments. 2017-08-22 Yury Norov <ynorov@caviumnetworks.com> Steve Ellcey <sellcey@cavium.com> * sysdeps/unix/sysv/linux/aarch64/bits/ipc.h (ipc_perm): Ifdef and pad the mode field for ILP32.
Comments
Hi Steve, On Tue, Aug 22, 2017 at 10:46:21AM -0700, Steve Ellcey wrote: > Here is another aarch64 ILP32 patch. The mode field in ipc_perm in ILP32 > should be a 16 bit field, not a 32 bit one. This was out-of-sync with what the > kernel had. This was causing sysvipc/test-sysvsem to fail in ILP32 mode. > This change is only needed for ILP32 so it doesn't need to go in until we add > that ABI but I am sending out for review and comments. > > 2017-08-22 Yury Norov <ynorov@caviumnetworks.com> > Steve Ellcey <sellcey@cavium.com> > > * sysdeps/unix/sysv/linux/aarch64/bits/ipc.h (ipc_perm): > Ifdef and pad the mode field for ILP32. > > > diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h b/sysdeps/unix/sysv/linu > x/aarch64/bits/ipc.h > index cd1f06e..cd05b74 100644 > --- a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > +++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > @@ -46,7 +46,12 @@ struct ipc_perm > __gid_t gid; /* Owner's group ID. */ > __uid_t cuid; /* Creator's user ID. */ > __gid_t cgid; /* Creator's group ID. */ > +#ifdef __LP64 This is my typo. It should be #ifdef __LP64__ > unsigned int mode; /* Read/write permission. */ > +#else > + unsigned short int mode; /* Read/write permission. */ > + unsigned short int __pad0; > +#endif > unsigned short int __seq; /* Sequence number. */ > unsigned short int __pad1; > __syscall_ulong_t __glibc_reserved1;
On 22/08/17 18:46, Steve Ellcey wrote: > Here is another aarch64 ILP32 patch. The mode field in ipc_perm in ILP32 > should be a 16 bit field, not a 32 bit one. This was out-of-sync with what the > kernel had. This was causing sysvipc/test-sysvsem to fail in ILP32 mode. > This change is only needed for ILP32 so it doesn't need to go in until we add > that ABI but I am sending out for review and comments. > > 2017-08-22 Yury Norov <ynorov@caviumnetworks.com> > Steve Ellcey <sellcey@cavium.com> > > * sysdeps/unix/sysv/linux/aarch64/bits/ipc.h (ipc_perm): > Ifdef and pad the mode field for ILP32. > > > diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h b/sysdeps/unix/sysv/linu > x/aarch64/bits/ipc.h > index cd1f06e..cd05b74 100644 > --- a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > +++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > @@ -46,7 +46,12 @@ struct ipc_perm > __gid_t gid; /* Owner's group ID. */ > __uid_t cuid; /* Creator's user ID. */ > __gid_t cgid; /* Creator's group ID. */ > +#ifdef __LP64 > unsigned int mode; /* Read/write permission. */ > +#else > + unsigned short int mode; /* Read/write permission. */ > + unsigned short int __pad0; > +#endif when did this happen? as far as i can tell staging/ilp32-4.12 branch in git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git still has unsigned int __kernel_mode_t in uapi, so this is an abi change compared to that branch. i guess it's for 32bit compat, but i'd like to see the kernel patch for this.
On Wed, Aug 23, 2017 at 10:55:02AM +0100, Szabolcs Nagy wrote: > On 22/08/17 18:46, Steve Ellcey wrote: > > Here is another aarch64 ILP32 patch. The mode field in ipc_perm in ILP32 > > should be a 16 bit field, not a 32 bit one. This was out-of-sync with what the > > kernel had. This was causing sysvipc/test-sysvsem to fail in ILP32 mode. > > This change is only needed for ILP32 so it doesn't need to go in until we add > > that ABI but I am sending out for review and comments. > > > > 2017-08-22 Yury Norov <ynorov@caviumnetworks.com> > > Steve Ellcey <sellcey@cavium.com> > > > > * sysdeps/unix/sysv/linux/aarch64/bits/ipc.h (ipc_perm): > > Ifdef and pad the mode field for ILP32. > > > > > > diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h b/sysdeps/unix/sysv/linu > > x/aarch64/bits/ipc.h > > index cd1f06e..cd05b74 100644 > > --- a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > > +++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h > > @@ -46,7 +46,12 @@ struct ipc_perm > > __gid_t gid; /* Owner's group ID. */ > > __uid_t cuid; /* Creator's user ID. */ > > __gid_t cgid; /* Creator's group ID. */ > > +#ifdef __LP64 > > unsigned int mode; /* Read/write permission. */ > > +#else > > + unsigned short int mode; /* Read/write permission. */ > > + unsigned short int __pad0; > > +#endif > > when did this happen? > > as far as i can tell staging/ilp32-4.12 branch in > git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git > still has unsigned int __kernel_mode_t in uapi, so this is > an abi change compared to that branch. > > i guess it's for 32bit compat, but i'd like to see the > kernel patch for this. It is there from the very beginning of ILP32 patches. The patch changes the ABI only for arm64/ilp32. LP64 remains untouched, and it works correct with and without ilp32 patches and this fix. To reproduce: struct semid_ds seminfo; static char name[] = "Hello"; key_t key = ftok (name, 'G'); semid = semget(key, 1, IPC_CREAT | IPC_EXCL | 0644); semctl (semid, 0, IPC_STAT, &seminfo); printf("%o\n", seminfo.sem_perm.mode); // bits 16-31 contain garbage On kernel side no fixes needed because arm64/ilp32 ipc requests are wired to compat layer, and the layout for structure is like this: arch/arm64/include/asm/compat.h: 246 struct compat_ipc64_perm { 247 compat_key_t key; 248 __compat_uid32_t uid; 249 __compat_gid32_t gid; 250 __compat_uid32_t cuid; 251 __compat_gid32_t cgid; 252 unsigned short mode; 253 unsigned short __pad1; 254 unsigned short seq; 255 unsigned short __pad2; 256 compat_ulong_t unused1; 257 compat_ulong_t unused2; 258 }; So I would prefer to treat it not as the change of ABI, bit the fix of ABI incompatibility on GLIBC side. Yury
On 23/08/17 11:10, Yury Norov wrote: > On Wed, Aug 23, 2017 at 10:55:02AM +0100, Szabolcs Nagy wrote: >> On 22/08/17 18:46, Steve Ellcey wrote: >>> @@ -46,7 +46,12 @@ struct ipc_perm >>> __gid_t gid; /* Owner's group ID. */ >>> __uid_t cuid; /* Creator's user ID. */ >>> __gid_t cgid; /* Creator's group ID. */ >>> +#ifdef __LP64 >>> unsigned int mode; /* Read/write permission. */ >>> +#else >>> + unsigned short int mode; /* Read/write permission. */ >>> + unsigned short int __pad0; >>> +#endif >> >> when did this happen? >> >> as far as i can tell staging/ilp32-4.12 branch in >> git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git >> still has unsigned int __kernel_mode_t in uapi, so this is >> an abi change compared to that branch. >> >> i guess it's for 32bit compat, but i'd like to see the >> kernel patch for this. > > It is there from the very beginning of ILP32 patches. The patch changes > the ABI only for arm64/ilp32. LP64 remains untouched, and it works correct > with and without ilp32 patches and this fix. > > To reproduce: > struct semid_ds seminfo; > static char name[] = "Hello"; > > key_t key = ftok (name, 'G'); > semid = semget(key, 1, IPC_CREAT | IPC_EXCL | 0644); > semctl (semid, 0, IPC_STAT, &seminfo); > printf("%o\n", seminfo.sem_perm.mode); // bits 16-31 contain garbage > > On kernel side no fixes needed because arm64/ilp32 ipc requests are > wired to compat layer, and the layout for structure is like this: > arch/arm64/include/asm/compat.h: > 246 struct compat_ipc64_perm { > 247 compat_key_t key; > 248 __compat_uid32_t uid; > 249 __compat_gid32_t gid; > 250 __compat_uid32_t cuid; > 251 __compat_gid32_t cgid; > 252 unsigned short mode; > 253 unsigned short __pad1; we have to decide if mode_t is unsigned int or short on ilp32, changing just the ipc_perm struct in libc is nonconforming: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html (there are some existing conformance issues like that in glibc/linux but we should try to avoid introducing new ones) i think the ilp32 linux uapi should typedef __kernel_mode_t to unsigned short, but i don't know the effect of that on the kernel, so please discuss this with the kernel folks.
On 23/08/17 11:37, Szabolcs Nagy wrote: > we have to decide if mode_t is unsigned int or short on ilp32, > changing just the ipc_perm struct in libc is nonconforming: > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html > > (there are some existing conformance issues like that in > glibc/linux but we should try to avoid introducing new ones) > > i think the ilp32 linux uapi should typedef __kernel_mode_t > to unsigned short, but i don't know the effect of that on > the kernel, so please discuss this with the kernel folks. > hm it seems to me that a mode_t change would be very intrusive.. can we keep the ipc_perm mode field unsigned int and do endian fixup/zero pad in the syscall interface?
On Wed, Aug 23, 2017 at 11:37:32AM +0100, Szabolcs Nagy wrote: > On 23/08/17 11:10, Yury Norov wrote: > > On Wed, Aug 23, 2017 at 10:55:02AM +0100, Szabolcs Nagy wrote: > >> On 22/08/17 18:46, Steve Ellcey wrote: > >>> @@ -46,7 +46,12 @@ struct ipc_perm > >>> __gid_t gid; /* Owner's group ID. */ > >>> __uid_t cuid; /* Creator's user ID. */ > >>> __gid_t cgid; /* Creator's group ID. */ > >>> +#ifdef __LP64 > >>> unsigned int mode; /* Read/write permission. */ > >>> +#else > >>> + unsigned short int mode; /* Read/write permission. */ > >>> + unsigned short int __pad0; > >>> +#endif > >> > >> when did this happen? > >> > >> as far as i can tell staging/ilp32-4.12 branch in > >> git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git > >> still has unsigned int __kernel_mode_t in uapi, so this is > >> an abi change compared to that branch. > >> > >> i guess it's for 32bit compat, but i'd like to see the > >> kernel patch for this. > > > > It is there from the very beginning of ILP32 patches. The patch changes > > the ABI only for arm64/ilp32. LP64 remains untouched, and it works correct > > with and without ilp32 patches and this fix. > > > > To reproduce: > > struct semid_ds seminfo; > > static char name[] = "Hello"; > > > > key_t key = ftok (name, 'G'); > > semid = semget(key, 1, IPC_CREAT | IPC_EXCL | 0644); > > semctl (semid, 0, IPC_STAT, &seminfo); > > printf("%o\n", seminfo.sem_perm.mode); // bits 16-31 contain garbage > > > > On kernel side no fixes needed because arm64/ilp32 ipc requests are > > wired to compat layer, and the layout for structure is like this: > > arch/arm64/include/asm/compat.h: > > 246 struct compat_ipc64_perm { > > 247 compat_key_t key; > > 248 __compat_uid32_t uid; > > 249 __compat_gid32_t gid; > > 250 __compat_uid32_t cuid; > > 251 __compat_gid32_t cgid; > > 252 unsigned short mode; > > 253 unsigned short __pad1; > > we have to decide if mode_t is unsigned int or short on ilp32, > changing just the ipc_perm struct in libc is nonconforming: > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html > > (there are some existing conformance issues like that in > glibc/linux but we should try to avoid introducing new ones) > > i think the ilp32 linux uapi should typedef __kernel_mode_t > to unsigned short, but i don't know the effect of that on > the kernel, so please discuss this with the kernel folks. The __mode_t is u32 for all glibc ports including arm64, but the problem is that the declaration of struct ipc_perm in sysdeps/unix/sysv/linux/bits/ipc.h doesn't use __mode_t type, but unsigned long or short. On kernel side, __kernel_mode_t is already defined, and for arm64 it is u32. And like for glibc, compat layer doesn't use __kernel_mode_t, but unsigned short. (For arm, the __kernel_mode_t is unsigned short, while __MODE_T_TYPE on glibc side is still __U32_TYPE. I think this is wrong...) So for me it looks like both glibc and kernel ignore POSIX standard in this case: declare mode not as mode_t type in the ipc_perm structure. And this is the root of problem. Fixing it is non-trivial task because mode_t is not unsigned short or long in general, and so other structures will be affected. For arm64/ilp32, we can continue as-is on glibc side, but it will require rework on kernel side. If it was only little-endian code, we would simply zero pad1, and it would be enough. But there is big-endian support for arm64/ilp32, and therefore we need to introduce wrappers to swap half-words. I can prepare RFC that does it, but I think that the problem is generic, and so the generic fix required. Yury
On 23/08/2017 08:31, Yury Norov wrote: > On Wed, Aug 23, 2017 at 11:37:32AM +0100, Szabolcs Nagy wrote: >> On 23/08/17 11:10, Yury Norov wrote: >>> On Wed, Aug 23, 2017 at 10:55:02AM +0100, Szabolcs Nagy wrote: >>>> On 22/08/17 18:46, Steve Ellcey wrote: >>>>> @@ -46,7 +46,12 @@ struct ipc_perm >>>>> __gid_t gid; /* Owner's group ID. */ >>>>> __uid_t cuid; /* Creator's user ID. */ >>>>> __gid_t cgid; /* Creator's group ID. */ >>>>> +#ifdef __LP64 >>>>> unsigned int mode; /* Read/write permission. */ >>>>> +#else >>>>> + unsigned short int mode; /* Read/write permission. */ >>>>> + unsigned short int __pad0; >>>>> +#endif >>>> >>>> when did this happen? >>>> >>>> as far as i can tell staging/ilp32-4.12 branch in >>>> git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git >>>> still has unsigned int __kernel_mode_t in uapi, so this is >>>> an abi change compared to that branch. >>>> >>>> i guess it's for 32bit compat, but i'd like to see the >>>> kernel patch for this. >>> >>> It is there from the very beginning of ILP32 patches. The patch changes >>> the ABI only for arm64/ilp32. LP64 remains untouched, and it works correct >>> with and without ilp32 patches and this fix. >>> >>> To reproduce: >>> struct semid_ds seminfo; >>> static char name[] = "Hello"; >>> >>> key_t key = ftok (name, 'G'); >>> semid = semget(key, 1, IPC_CREAT | IPC_EXCL | 0644); >>> semctl (semid, 0, IPC_STAT, &seminfo); >>> printf("%o\n", seminfo.sem_perm.mode); // bits 16-31 contain garbage >>> >>> On kernel side no fixes needed because arm64/ilp32 ipc requests are >>> wired to compat layer, and the layout for structure is like this: >>> arch/arm64/include/asm/compat.h: >>> 246 struct compat_ipc64_perm { >>> 247 compat_key_t key; >>> 248 __compat_uid32_t uid; >>> 249 __compat_gid32_t gid; >>> 250 __compat_uid32_t cuid; >>> 251 __compat_gid32_t cgid; >>> 252 unsigned short mode; >>> 253 unsigned short __pad1; >> >> we have to decide if mode_t is unsigned int or short on ilp32, >> changing just the ipc_perm struct in libc is nonconforming: >> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html >> >> (there are some existing conformance issues like that in >> glibc/linux but we should try to avoid introducing new ones) >> >> i think the ilp32 linux uapi should typedef __kernel_mode_t >> to unsigned short, but i don't know the effect of that on >> the kernel, so please discuss this with the kernel folks. > > The __mode_t is u32 for all glibc ports including arm64, but the problem is > that the declaration of struct ipc_perm in sysdeps/unix/sysv/linux/bits/ipc.h > doesn't use __mode_t type, but unsigned long or short. > > On kernel side, __kernel_mode_t is already defined, and for arm64 it > is u32. And like for glibc, compat layer doesn't use __kernel_mode_t, > but unsigned short. > > (For arm, the __kernel_mode_t is unsigned short, while __MODE_T_TYPE > on glibc side is still __U32_TYPE. I think this is wrong...) > > So for me it looks like both glibc and kernel ignore POSIX standard in > this case: declare mode not as mode_t type in the ipc_perm structure. > And this is the root of problem. Fixing it is non-trivial task because > mode_t is not unsigned short or long in general, and so other structures > will be affected. The old SysV kernel ABI interface is messy and kernel does not help us in this manner. The '__kernel_mode_t' is defined on kernel sources as: arch/arm/include/uapi/asm/posix_types.h 22 typedef unsigned short __kernel_mode_t; arch/m68k/include/uapi/asm/posix_types.h 10 typedef unsigned short __kernel_mode_t; arch/microblaze/include/uapi/asm/posix_types.h 4 typedef unsigned short __kernel_mode_t; arch/parisc/include/uapi/asm/posix_types.h 11 typedef unsigned short __kernel_mode_t; arch/s390/include/uapi/asm/posix_types.h 25 typedef unsigned short __kernel_mode_t; arch/s390/include/uapi/asm/posix_types.h 34 typedef unsigned int __kernel_mode_t; arch/sh/include/uapi/asm/posix_types_32.h 4 typedef unsigned short __kernel_mode_t; arch/sh/include/uapi/asm/posix_types_64.h 4 typedef unsigned short __kernel_mode_t; arch/sparc/include/uapi/asm/posix_types.h 36 typedef unsigned short __kernel_mode_t; arch/x86/include/uapi/asm/posix_types_32.h 10 typedef unsigned short __kernel_mode_t; include/uapi/asm-generic/posix_types.h 23 typedef unsigned int __kernel_mode_t; (I excluded non glibc supported architectures) Basically old 32-bit ABIs uses unsigned short and newer ABI and 64 bits uses unsigned int. Also, to avoid adding another entrypoint for these old interfaces kernel added IPC_64 to indicate support to 32-bit UIDs: arch/aarch64/bits/ipc.h 14 #define IPC_64 0 arch/generic/bits/ipc.h 13 #define IPC_64 0x100 arch/mips64/bits/ipc.h 14 #define IPC_64 0x100 arch/powerpc/bits/ipc.h 14 #define IPC_64 0x100 arch/powerpc64/bits/ipc.h 14 #define IPC_64 0x100 arch/s390x/bits/ipc.h 14 #define IPC_64 0x100 arch/x32/bits/ipc.h 13 #define IPC_64 0 arch/x86_64/bits/ipc.h 13 #define IPC_64 0 (I excluded non glibc supported architectures) However IPC_64 only make sense for ABI that used to provide 16 bits uids and then added supported to 32 bits (that's why x86_64/AARCh64 IPC_64 is set to 0). This is kinda messy because for newer ports is exactly the contrary (default to a 0x100). Anyhow, on current GLIBC we use only 32 bits uids as default (even for old ABIs): io/fcntl.h 50 typedef __mode_t mode_t; io/sys/stat.h 59 typedef __mode_t mode_t; misc/sys/mman.h 37 typedef __mode_t mode_t; posix/sys/types.h 70 typedef __mode_t mode_t; sysvipc/sys/ipc.h 38 typedef __mode_t mode_t; posix/bits/types.h 138 __STD_TYPE __MODE_T_TYPE __mode_t; bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE mach/hurd/bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE linux/alpha/bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE linux/generic/bits/typesizes.h 35 #define __MODE_T_TYPE __U32_TYPE linux/s390/bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE linux/sparc/bits/typesizes.h 34 #define __MODE_T_TYPE __U32_TYPE linux/x86/bits/typesizes.h 43 #define __MODE_T_TYPE __U32_TYPE Old uid 16 bits is used solely on compat symbols. And that's why we explicit define the types to unsigned/short instead of using mode_t. I think it feasible to change ipc_perm to use 32 mode_t for all architectures, but it means that we will need to actually make a struct copy on architectures with 16 bits uids that do not support IPC_64 (and unfortunately we still have some). Now for ILP32 I see that it tries to use internal ARM code as much as possible, so using 32-bits mode_t might require a lot of internal kernel refactor. Would it be possible at least to have IPC_64 support then? I presume this would require add support for ARM as well. > > For arm64/ilp32, we can continue as-is on glibc side, but it will > require rework on kernel side. If it was only little-endian code, we > would simply zero pad1, and it would be enough. But there is big-endian > support for arm64/ilp32, and therefore we need to introduce wrappers > to swap half-words. > > I can prepare RFC that does it, but I think that the problem is > generic, and so the generic fix required. > > Yury >
On 23/08/17 12:31, Yury Norov wrote: > On Wed, Aug 23, 2017 at 11:37:32AM +0100, Szabolcs Nagy wrote: >> On 23/08/17 11:10, Yury Norov wrote: >>> On Wed, Aug 23, 2017 at 10:55:02AM +0100, Szabolcs Nagy wrote: >>>> On 22/08/17 18:46, Steve Ellcey wrote: >>>>> @@ -46,7 +46,12 @@ struct ipc_perm >>>>> __gid_t gid; /* Owner's group ID. */ >>>>> __uid_t cuid; /* Creator's user ID. */ >>>>> __gid_t cgid; /* Creator's group ID. */ >>>>> +#ifdef __LP64 >>>>> unsigned int mode; /* Read/write permission. */ >>>>> +#else >>>>> + unsigned short int mode; /* Read/write permission. */ >>>>> + unsigned short int __pad0; >>>>> +#endif >>>> >>>> when did this happen? >>>> >>>> as far as i can tell staging/ilp32-4.12 branch in >>>> git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git >>>> still has unsigned int __kernel_mode_t in uapi, so this is >>>> an abi change compared to that branch. >>>> >>>> i guess it's for 32bit compat, but i'd like to see the >>>> kernel patch for this. >>> >>> It is there from the very beginning of ILP32 patches. The patch changes >>> the ABI only for arm64/ilp32. LP64 remains untouched, and it works correct >>> with and without ilp32 patches and this fix. >>> >>> To reproduce: >>> struct semid_ds seminfo; >>> static char name[] = "Hello"; >>> >>> key_t key = ftok (name, 'G'); >>> semid = semget(key, 1, IPC_CREAT | IPC_EXCL | 0644); >>> semctl (semid, 0, IPC_STAT, &seminfo); >>> printf("%o\n", seminfo.sem_perm.mode); // bits 16-31 contain garbage >>> >>> On kernel side no fixes needed because arm64/ilp32 ipc requests are >>> wired to compat layer, and the layout for structure is like this: >>> arch/arm64/include/asm/compat.h: >>> 246 struct compat_ipc64_perm { >>> 247 compat_key_t key; >>> 248 __compat_uid32_t uid; >>> 249 __compat_gid32_t gid; >>> 250 __compat_uid32_t cuid; >>> 251 __compat_gid32_t cgid; >>> 252 unsigned short mode; >>> 253 unsigned short __pad1; >> >> we have to decide if mode_t is unsigned int or short on ilp32, >> changing just the ipc_perm struct in libc is nonconforming: >> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html >> >> (there are some existing conformance issues like that in >> glibc/linux but we should try to avoid introducing new ones) >> >> i think the ilp32 linux uapi should typedef __kernel_mode_t >> to unsigned short, but i don't know the effect of that on >> the kernel, so please discuss this with the kernel folks. > > The __mode_t is u32 for all glibc ports including arm64, but the problem is > that the declaration of struct ipc_perm in sysdeps/unix/sysv/linux/bits/ipc.h > doesn't use __mode_t type, but unsigned long or short. > > On kernel side, __kernel_mode_t is already defined, and for arm64 it > is u32. And like for glibc, compat layer doesn't use __kernel_mode_t, > but unsigned short. > > (For arm, the __kernel_mode_t is unsigned short, while __MODE_T_TYPE > on glibc side is still __U32_TYPE. I think this is wrong...) > > So for me it looks like both glibc and kernel ignore POSIX standard in > this case: declare mode not as mode_t type in the ipc_perm structure. > And this is the root of problem. Fixing it is non-trivial task because > mode_t is not unsigned short or long in general, and so other structures > will be affected. > ok, it seems it's one of the posix conformance issues that affect other targets too https://sourceware.org/bugzilla/show_bug.cgi?id=18231 so it's ok to have this issue on ilp32 too. > For arm64/ilp32, we can continue as-is on glibc side, but it will > require rework on kernel side. If it was only little-endian code, we > would simply zero pad1, and it would be enough. But there is big-endian > support for arm64/ilp32, and therefore we need to introduce wrappers > to swap half-words. > > I can prepare RFC that does it, but I think that the problem is > generic, and so the generic fix required. > > Yury >
On 23/08/2017 09:42, Szabolcs Nagy wrote: > On 23/08/17 12:31, Yury Norov wrote: >> On Wed, Aug 23, 2017 at 11:37:32AM +0100, Szabolcs Nagy wrote: >>> On 23/08/17 11:10, Yury Norov wrote: >>>> On Wed, Aug 23, 2017 at 10:55:02AM +0100, Szabolcs Nagy wrote: >>>>> On 22/08/17 18:46, Steve Ellcey wrote: >>>>>> @@ -46,7 +46,12 @@ struct ipc_perm >>>>>> __gid_t gid; /* Owner's group ID. */ >>>>>> __uid_t cuid; /* Creator's user ID. */ >>>>>> __gid_t cgid; /* Creator's group ID. */ >>>>>> +#ifdef __LP64 >>>>>> unsigned int mode; /* Read/write permission. */ >>>>>> +#else >>>>>> + unsigned short int mode; /* Read/write permission. */ >>>>>> + unsigned short int __pad0; >>>>>> +#endif >>>>> >>>>> when did this happen? >>>>> >>>>> as far as i can tell staging/ilp32-4.12 branch in >>>>> git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git >>>>> still has unsigned int __kernel_mode_t in uapi, so this is >>>>> an abi change compared to that branch. >>>>> >>>>> i guess it's for 32bit compat, but i'd like to see the >>>>> kernel patch for this. >>>> >>>> It is there from the very beginning of ILP32 patches. The patch changes >>>> the ABI only for arm64/ilp32. LP64 remains untouched, and it works correct >>>> with and without ilp32 patches and this fix. >>>> >>>> To reproduce: >>>> struct semid_ds seminfo; >>>> static char name[] = "Hello"; >>>> >>>> key_t key = ftok (name, 'G'); >>>> semid = semget(key, 1, IPC_CREAT | IPC_EXCL | 0644); >>>> semctl (semid, 0, IPC_STAT, &seminfo); >>>> printf("%o\n", seminfo.sem_perm.mode); // bits 16-31 contain garbage >>>> >>>> On kernel side no fixes needed because arm64/ilp32 ipc requests are >>>> wired to compat layer, and the layout for structure is like this: >>>> arch/arm64/include/asm/compat.h: >>>> 246 struct compat_ipc64_perm { >>>> 247 compat_key_t key; >>>> 248 __compat_uid32_t uid; >>>> 249 __compat_gid32_t gid; >>>> 250 __compat_uid32_t cuid; >>>> 251 __compat_gid32_t cgid; >>>> 252 unsigned short mode; >>>> 253 unsigned short __pad1; >>> >>> we have to decide if mode_t is unsigned int or short on ilp32, >>> changing just the ipc_perm struct in libc is nonconforming: >>> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_ipc.h.html >>> >>> (there are some existing conformance issues like that in >>> glibc/linux but we should try to avoid introducing new ones) >>> >>> i think the ilp32 linux uapi should typedef __kernel_mode_t >>> to unsigned short, but i don't know the effect of that on >>> the kernel, so please discuss this with the kernel folks. >> >> The __mode_t is u32 for all glibc ports including arm64, but the problem is >> that the declaration of struct ipc_perm in sysdeps/unix/sysv/linux/bits/ipc.h >> doesn't use __mode_t type, but unsigned long or short. >> >> On kernel side, __kernel_mode_t is already defined, and for arm64 it >> is u32. And like for glibc, compat layer doesn't use __kernel_mode_t, >> but unsigned short. >> >> (For arm, the __kernel_mode_t is unsigned short, while __MODE_T_TYPE >> on glibc side is still __U32_TYPE. I think this is wrong...) >> >> So for me it looks like both glibc and kernel ignore POSIX standard in >> this case: declare mode not as mode_t type in the ipc_perm structure. >> And this is the root of problem. Fixing it is non-trivial task because >> mode_t is not unsigned short or long in general, and so other structures >> will be affected. >> > > ok, it seems it's one of the posix conformance issues > that affect other targets too > > https://sourceware.org/bugzilla/show_bug.cgi?id=18231 > > so it's ok to have this issue on ilp32 too. I think it is feasible to fix it on generic ipc.h and let the required architecture to redefine it as required for their own kernel ABIs. > >> For arm64/ilp32, we can continue as-is on glibc side, but it will >> require rework on kernel side. If it was only little-endian code, we >> would simply zero pad1, and it would be enough. But there is big-endian >> support for arm64/ilp32, and therefore we need to introduce wrappers >> to swap half-words. >> >> I can prepare RFC that does it, but I think that the problem is >> generic, and so the generic fix required. >> >> Yury >> >
diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h b/sysdeps/unix/sysv/linu x/aarch64/bits/ipc.h index cd1f06e..cd05b74 100644 --- a/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h +++ b/sysdeps/unix/sysv/linux/aarch64/bits/ipc.h @@ -46,7 +46,12 @@ struct ipc_perm      __gid_t gid; /* Owner's group ID.  */      __uid_t cuid; /* Creator's user ID.  */      __gid_t cgid; /* Creator's group ID.  */ +#ifdef __LP64      unsigned int mode; /* Read/write permission.  */ +#else +    unsigned short int mode; /* Read/write permission.  */ +    unsigned short int __pad0; +#endif      unsigned short int __seq; /* Sequence number.  */      unsigned short int __pad1;      __syscall_ulong_t __glibc_reserved1;