| Message ID | 1509958663-18737-30-git-send-email-linuxram@us.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc, mm: Memory Protection Keys | expand |
On 11/06/2017 12:57 AM, Ram Pai wrote: > Expose useful information for programs using memory protection keys. > Provide implementation for powerpc and x86. > > On a powerpc system with pkeys support, here is what is shown: > > $ head /sys/kernel/mm/protection_keys/* > ==> /sys/kernel/mm/protection_keys/disable_access_supported <== > true This is cute, but I don't think it should be part of the ABI. Put it in debugfs if you want it for cute tests. The stuff that this tells you can and should come from pkey_alloc() for the ABI. http://man7.org/linux/man-pages/man7/pkeys.7.html > Any application wanting to use protection keys needs to be able to > function without them. They might be unavailable because the > hardware that the application runs on does not support them, the > kernel code does not contain support, the kernel support has been > disabled, or because the keys have all been allocated, perhaps by a > library the application is using. It is recommended that > applications wanting to use protection keys should simply call > pkey_alloc(2) and test whether the call succeeds, instead of > attempting to detect support for the feature in any other way. Do you really not have standard way on ppc to say whether hardware features are supported by the kernel? For instance, how do you know if a given set of registers are known to and are being context-switched by the kernel?
On Mon, Dec 18, 2017 at 10:54:26AM -0800, Dave Hansen wrote: > On 11/06/2017 12:57 AM, Ram Pai wrote: > > Expose useful information for programs using memory protection keys. > > Provide implementation for powerpc and x86. > > > > On a powerpc system with pkeys support, here is what is shown: > > > > $ head /sys/kernel/mm/protection_keys/* > > ==> /sys/kernel/mm/protection_keys/disable_access_supported <== > > true > > This is cute, but I don't think it should be part of the ABI. Put it in thanks :) > debugfs if you want it for cute tests. The stuff that this tells you > can and should come from pkey_alloc() for the ABI. Applications can make system calls with different parameters and on failure determine indirectly that such a feature may not be available in the kernel/hardware. But from an application point of view, I think, it is a very clumsy/difficult way to determine that. For example, an application can keep making pkey_alloc() calls and count till the call fails, to determine the number of keys supported by the system. And then the application has to release those keys too. Too much side-effect just to determine a simple thing. Do we want the application to endure this pain? I think we should aim to provide sufficient API/ABI for the application to consume the feature efficiently, and not any more. I do not claim that the ABI exposed by this patch is sufficiently optimal. But I do believe it is tending towards it. currently the following ABI is exposed. a) total number of keys available in the system. This information may not be useful and can possibly be dropped. b) minimum number of keys available to the application. if libraries consumes a few, they could provide a library interface to the application informing the number available to the application. The library interface can leverage (b) to provide the information. c) types of disable-rights supported by keys. Helps the application to determine the types of disable-features available. This is helpful, otherwise the app has to make pkey_alloc() call with the corresponding parameter set and see if it suceeds or fails. Painful from an application point of view, in my opinion. > > http://man7.org/linux/man-pages/man7/pkeys.7.html > > > Any application wanting to use protection keys needs to be able to > > function without them. They might be unavailable because the > > hardware that the application runs on does not support them, the > > kernel code does not contain support, the kernel support has been > > disabled, or because the keys have all been allocated, perhaps by a > > library the application is using. It is recommended that > > applications wanting to use protection keys should simply call > > pkey_alloc(2) and test whether the call succeeds, instead of > > attempting to detect support for the feature in any other way. > > Do you really not have standard way on ppc to say whether hardware > features are supported by the kernel? For instance, how do you know if > a given set of registers are known to and are being context-switched by > the kernel? I think on x86 you look for some hardware registers to determine which hardware features are enabled by the kernel. We do not have generic support for something like that on ppc. The kernel looks at the device tree to determine what hardware features are available. But does not have mechanism to tell the hardware to track which of its features are currently enabled/used by the kernel; atleast not for the memory-key feature. RP
On 12/18/2017 02:18 PM, Ram Pai wrote: > b) minimum number of keys available to the application. > if libraries consumes a few, they could provide a library > interface to the application informing the number available to > the application. The library interface can leverage (b) to > provide the information. OK, let's see a real user of this including a few libraries. Then we'll put it in the kernel. > c) types of disable-rights supported by keys. > Helps the application to determine the types of disable-features > available. This is helpful, otherwise the app has to > make pkey_alloc() call with the corresponding parameter set > and see if it suceeds or fails. Painful from an application > point of view, in my opinion. Again, let's see a real-world use of this. How does it look? How does an app "fall back" if it can't set a restriction the way it wants to? Are we *sure* that such an interface makes sense? For instance, will it be possible for some keys to be execute-disable while other are only write-disable? > I think on x86 you look for some hardware registers to determine which > hardware features are enabled by the kernel. No, we use CPUID. It's a part of the ISA that's designed for enumerating CPU and (sometimes) OS support for CPU features. > We do not have generic support for something like that on ppc. > The kernel looks at the device tree to determine what hardware features > are available. But does not have mechanism to tell the hardware to track > which of its features are currently enabled/used by the kernel; atleast > not for the memory-key feature. Bummer. You're missing out. But, you could still do this with a syscall. "Hey, kernel, do you support this feature?"
On Mon, Dec 18, 2017 at 02:28:14PM -0800, Dave Hansen wrote: > On 12/18/2017 02:18 PM, Ram Pai wrote: > > b) minimum number of keys available to the application. > > if libraries consumes a few, they could provide a library > > interface to the application informing the number available to > > the application. The library interface can leverage (b) to > > provide the information. > > OK, let's see a real user of this including a few libraries. Then we'll > put it in the kernel. > > > c) types of disable-rights supported by keys. > > Helps the application to determine the types of disable-features > > available. This is helpful, otherwise the app has to > > make pkey_alloc() call with the corresponding parameter set > > and see if it suceeds or fails. Painful from an application > > point of view, in my opinion. > > Again, let's see a real-world use of this. How does it look? How does > an app "fall back" if it can't set a restriction the way it wants to? > > Are we *sure* that such an interface makes sense? For instance, will it > be possible for some keys to be execute-disable while other are only > write-disable? Can it be on x86? its not possible on ppc. the keys can *all* be the-same-attributes-disable all the time. However you are right. Its conceivable that some arch could provide a feature where it can be x-attribute-disable for key 'a' and y-attribute-disable for key 'b'. But than its a bit of a headache for an application to consume such a feature. Ben: I recall you requesting this feature. Thoughts? > > > I think on x86 you look for some hardware registers to determine > > which hardware features are enabled by the kernel. > > No, we use CPUID. It's a part of the ISA that's designed for > enumerating CPU and (sometimes) OS support for CPU features. > > > We do not have generic support for something like that on ppc. The > > kernel looks at the device tree to determine what hardware features > > are available. But does not have mechanism to tell the hardware to > > track which of its features are currently enabled/used by the > > kernel; atleast not for the memory-key feature. > > Bummer. You're missing out. > > But, you could still do this with a syscall. "Hey, kernel, do you > support this feature?" or do powerpc specific sysfs interface. or a debugfs interface. RP
On Mon, Dec 18, 2017 at 03:15:51PM -0800, Ram Pai wrote: > On Mon, Dec 18, 2017 at 02:28:14PM -0800, Dave Hansen wrote: > > On 12/18/2017 02:18 PM, Ram Pai wrote: > > > b) minimum number of keys available to the application. > > > if libraries consumes a few, they could provide a library > > > interface to the application informing the number available to > > > the application. The library interface can leverage (b) to > > > provide the information. > > > > OK, let's see a real user of this including a few libraries. Then we'll > > put it in the kernel. > > > > > c) types of disable-rights supported by keys. > > > Helps the application to determine the types of disable-features > > > available. This is helpful, otherwise the app has to > > > make pkey_alloc() call with the corresponding parameter set > > > and see if it suceeds or fails. Painful from an application > > > point of view, in my opinion. > > > > Again, let's see a real-world use of this. How does it look? How does > > an app "fall back" if it can't set a restriction the way it wants to? > > > > Are we *sure* that such an interface makes sense? For instance, will it > > be possible for some keys to be execute-disable while other are only > > write-disable? > > Can it be on x86? > > its not possible on ppc. the keys can *all* be the-same-attributes-disable all the > time. > > However you are right. Its conceivable that some arch could provide a > feature where it can be x-attribute-disable for key 'a' and > y-attribute-disable for key 'b'. But than its a bit of a headache > for an application to consume such a feature. > > Ben: I recall you requesting this feature. Thoughts? > > > > > > I think on x86 you look for some hardware registers to determine > > > which hardware features are enabled by the kernel. > > > > No, we use CPUID. It's a part of the ISA that's designed for > > enumerating CPU and (sometimes) OS support for CPU features. > > > > > We do not have generic support for something like that on ppc. The > > > kernel looks at the device tree to determine what hardware features > > > are available. But does not have mechanism to tell the hardware to > > > track which of its features are currently enabled/used by the > > > kernel; atleast not for the memory-key feature. > > > > Bummer. You're missing out. > > > > But, you could still do this with a syscall. "Hey, kernel, do you > > support this feature?" > > or do powerpc specific sysfs interface. > or a debugfs interface. getauxval(3) ? With AT_HWCAP or HWCAP2 as parameter already gives information about features supported by the hardware and the kernel. Taking one bit to expose the availability of protection keys to applications does not look impossible. Do I miss something obvious? Gabriel
Dave Hansen <dave.hansen@intel.com> writes: > On 11/06/2017 12:57 AM, Ram Pai wrote: >> Expose useful information for programs using memory protection keys. >> Provide implementation for powerpc and x86. >> >> On a powerpc system with pkeys support, here is what is shown: >> >> $ head /sys/kernel/mm/protection_keys/* >> ==> /sys/kernel/mm/protection_keys/disable_access_supported <== >> true > > This is cute, but I don't think it should be part of the ABI. Put it in > debugfs if you want it for cute tests. The stuff that this tells you > can and should come from pkey_alloc() for the ABI. Yeah I agree this is not sysfs material. In particular the total/usable numbers are completely useless vs other threads allocating pkeys out from under you. > http://man7.org/linux/man-pages/man7/pkeys.7.html > >> Any application wanting to use protection keys needs to be able to >> function without them. They might be unavailable because the >> hardware that the application runs on does not support them, the >> kernel code does not contain support, the kernel support has been >> disabled, or because the keys have all been allocated, perhaps by a >> library the application is using. It is recommended that >> applications wanting to use protection keys should simply call >> pkey_alloc(2) and test whether the call succeeds, instead of >> attempting to detect support for the feature in any other way. > > Do you really not have standard way on ppc to say whether hardware > features are supported by the kernel? For instance, how do you know if > a given set of registers are known to and are being context-switched by > the kernel? Yes we do, we emit feature bits in the AT_HWCAP entry of the aux vector, same as some other architectures. But I don't see the need to use a feature bit for pkeys. If they're not supported then pkey_alloc() will just always fail. Apps have to handle that anyway because keys are a finite resource. cheers
On Tue, Dec 19, 2017 at 09:31:22AM +0100, Gabriel Paubert wrote: > On Mon, Dec 18, 2017 at 03:15:51PM -0800, Ram Pai wrote: > > On Mon, Dec 18, 2017 at 02:28:14PM -0800, Dave Hansen wrote: > > > On 12/18/2017 02:18 PM, Ram Pai wrote: > > > ....snip... > > > > I think on x86 you look for some hardware registers to determine > > > > which hardware features are enabled by the kernel. > > > > > > No, we use CPUID. It's a part of the ISA that's designed for > > > enumerating CPU and (sometimes) OS support for CPU features. > > > > > > > We do not have generic support for something like that on ppc. The > > > > kernel looks at the device tree to determine what hardware features > > > > are available. But does not have mechanism to tell the hardware to > > > > track which of its features are currently enabled/used by the > > > > kernel; atleast not for the memory-key feature. > > > > > > Bummer. You're missing out. > > > > > > But, you could still do this with a syscall. "Hey, kernel, do you > > > support this feature?" > > > > or do powerpc specific sysfs interface. > > or a debugfs interface. > > getauxval(3) ? > > With AT_HWCAP or HWCAP2 as parameter already gives information about > features supported by the hardware and the kernel. > > Taking one bit to expose the availability of protection keys to > applications does not look impossible. > > Do I miss something obvious? No. I am told this is possible aswell. RP
On Tue, Dec 19, 2017 at 09:50:24PM +1100, Michael Ellerman wrote: > Dave Hansen <dave.hansen@intel.com> writes: > > > On 11/06/2017 12:57 AM, Ram Pai wrote: > >> Expose useful information for programs using memory protection keys. > >> Provide implementation for powerpc and x86. > >> > >> On a powerpc system with pkeys support, here is what is shown: > >> > >> $ head /sys/kernel/mm/protection_keys/* > >> ==> /sys/kernel/mm/protection_keys/disable_access_supported <== > >> true > > > > This is cute, but I don't think it should be part of the ABI. Put it in > > debugfs if you want it for cute tests. The stuff that this tells you > > can and should come from pkey_alloc() for the ABI. > > Yeah I agree this is not sysfs material. > > In particular the total/usable numbers are completely useless vs other > threads allocating pkeys out from under you. The usable number is the minimum number of keys available for use by the application, not the number of keys **currently** available. Its a static number. I am dropping this patch. We can revisit this when a clear request for such a feature emerges. > > > > >> Any application wanting to use protection keys needs to be able to > >> function without them. They might be unavailable because the > >> hardware that the application runs on does not support them, the > >> kernel code does not contain support, the kernel support has been > >> disabled, or because the keys have all been allocated, perhaps by a > >> library the application is using. It is recommended that > >> applications wanting to use protection keys should simply call > >> pkey_alloc(2) and test whether the call succeeds, instead of > >> attempting to detect support for the feature in any other way. > > > > Do you really not have standard way on ppc to say whether hardware > > features are supported by the kernel? For instance, how do you know if > > a given set of registers are known to and are being context-switched by > > the kernel? > > Yes we do, we emit feature bits in the AT_HWCAP entry of the aux vector, > same as some other architectures. Ah. I was not aware of this. Thanks, RP
On Mon, 2017-12-18 at 14:28 -0800, Dave Hansen wrote: > > We do not have generic support for something like that on ppc. > > The kernel looks at the device tree to determine what hardware features > > are available. But does not have mechanism to tell the hardware to track > > which of its features are currently enabled/used by the kernel; atleast > > not for the memory-key feature. > > Bummer. You're missing out. > > But, you could still do this with a syscall. "Hey, kernel, do you > support this feature?" I'm not sure I understand Ram's original (quoted) point, but informing userspace of CPU features is what AT_HWCAP's are about. Ben.
On Wed, Dec 20, 2017 at 08:34:56AM +1100, Benjamin Herrenschmidt wrote: > On Mon, 2017-12-18 at 14:28 -0800, Dave Hansen wrote: > > > We do not have generic support for something like that on ppc. > > > The kernel looks at the device tree to determine what hardware features > > > are available. But does not have mechanism to tell the hardware to track > > > which of its features are currently enabled/used by the kernel; atleast > > > not for the memory-key feature. > > > > Bummer. You're missing out. > > > > But, you could still do this with a syscall. "Hey, kernel, do you > > support this feature?" > > I'm not sure I understand Ram's original (quoted) point, but informing > userspace of CPU features is what AT_HWCAP's are about. Ben, my original point was -- we developed this patch to satisfy a concern you raised back on July 11th; cut-n-pasted below. ------------------------------------------------------------------- That leads to the question... How do you tell userspace. (apologies if I missed that in an existing patch in the series) How do we inform userspace of the key capabilities ? There are at least two things userspace may want to know already: - What protection bits are supported for a key - How many keys exist - Which keys are available for use by userspace. On PowerPC, the kernel can reserve some keys for itself, so can the hypervisor. In fact, they do. -------------------------------------------------------------------- The argument against this patch is -- it should not be baked into the ABI as yet, since we do not have clarity on what applications need. As it stands today the only way to figure out the information from userspace is by probing the kernel through calls to sys_pkey_alloc(). AT_HWCAP can be used, but that will certainly not be capable of providing all the information that userspace might expect. Your thoughts? RP
On Wed, 2017-12-20 at 09:50 -0800, Ram Pai wrote: > The argument against this patch is -- it should not be baked into > the ABI as yet, since we do not have clarity on what applications need. > > As it stands today the only way to figure out the information from > userspace is by probing the kernel through calls to sys_pkey_alloc(). > > AT_HWCAP can be used, but that will certainly not be capable of > providing all the information that userspace might expect. > > Your thoughts? Well, there's one well known application wanting that whole keys business, so why not ask them what works for them ? In the meantime, that shouldn't block the rest of the patches. Cheers, Ben.
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h index 333fb28..6d70b1a 100644 --- a/arch/powerpc/include/asm/pkeys.h +++ b/arch/powerpc/include/asm/pkeys.h @@ -237,6 +237,8 @@ static inline void pkey_mmu_values(int total_data, int total_execute) pkeys_total = total_data; } +extern bool arch_supports_pkeys(int cap); +extern unsigned int arch_usable_pkeys(void); extern void thread_pkey_regs_save(struct thread_struct *thread); extern void thread_pkey_regs_restore(struct thread_struct *new_thread, struct thread_struct *old_thread); diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c index 2612f61..7e8468f 100644 --- a/arch/powerpc/mm/pkeys.c +++ b/arch/powerpc/mm/pkeys.c @@ -421,6 +421,30 @@ bool arch_vma_access_permitted(struct vm_area_struct *vma, bool write, return pkey_access_permitted(vma_pkey(vma), write, execute); } +unsigned int arch_usable_pkeys(void) +{ + unsigned int reserved; + + if (static_branch_likely(&pkey_disabled)) + return 0; + + /* Reserve one more to account for the execute-only pkey. */ + reserved = hweight32(initial_allocation_mask) + 1; + + return pkeys_total > reserved ? pkeys_total - reserved : 0; +} + +bool arch_supports_pkeys(int cap) +{ + if (static_branch_likely(&pkey_disabled)) + return false; + + if (cap & PKEY_DISABLE_EXECUTE) + return pkey_execute_disable_supported; + + return (cap & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE)); +} + long sys_pkey_modify(int pkey, unsigned long new_val) { bool ret; diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h index 6699fc4..e3efabb 100644 --- a/arch/x86/include/asm/mmu_context.h +++ b/arch/x86/include/asm/mmu_context.h @@ -129,6 +129,8 @@ static inline void switch_ldt(struct mm_struct *prev, struct mm_struct *next) void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk); +#define PKEY_INITIAL_ALLOCATION_MAP 1 + static inline int init_new_context(struct task_struct *tsk, struct mm_struct *mm) { @@ -138,7 +140,7 @@ static inline int init_new_context(struct task_struct *tsk, #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS if (cpu_feature_enabled(X86_FEATURE_OSPKE)) { /* pkey 0 is the default and always allocated */ - mm->context.pkey_allocation_map = 0x1; + mm->context.pkey_allocation_map = PKEY_INITIAL_ALLOCATION_MAP; /* -1 means unallocated or invalid */ mm->context.execute_only_pkey = -1; } diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h index f6c287b..6807288 100644 --- a/arch/x86/include/asm/pkeys.h +++ b/arch/x86/include/asm/pkeys.h @@ -106,5 +106,6 @@ extern int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey, unsigned long init_val); extern void copy_init_pkru_to_fpregs(void); +extern unsigned int arch_usable_pkeys(void); #endif /*_ASM_X86_PKEYS_H */ diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c index d7bc0ee..3083a59 100644 --- a/arch/x86/mm/pkeys.c +++ b/arch/x86/mm/pkeys.c @@ -122,6 +122,15 @@ int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot, int pkey return vma_pkey(vma); } +unsigned int arch_usable_pkeys(void) +{ + /* Reserve one more to account for the execute-only pkey. */ + unsigned int reserved = (boot_cpu_has(X86_FEATURE_OSPKE) ? + hweight32(PKEY_INITIAL_ALLOCATION_MAP) : 0) + 1; + + return arch_max_pkey() > reserved ? arch_max_pkey() - reserved : 0; +} + #define PKRU_AD_KEY(pkey) (PKRU_AD_BIT << ((pkey) * PKRU_BITS_PER_PKEY)) /* diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h index 3ca2e44..0784f20 100644 --- a/include/linux/pkeys.h +++ b/include/linux/pkeys.h @@ -11,6 +11,7 @@ #define arch_max_pkey() (1) #define execute_only_pkey(mm) (0) #define arch_override_mprotect_pkey(vma, prot, pkey) (0) +#define arch_usable_pkeys() (0) #define PKEY_DEDICATED_EXECUTE_ONLY 0 #define ARCH_VM_PKEY_FLAGS 0 @@ -43,7 +44,6 @@ static inline bool arch_pkeys_enabled(void) static inline void copy_init_pkru_to_fpregs(void) { } - #endif /* ! CONFIG_ARCH_HAS_PKEYS */ #endif /* _LINUX_PKEYS_H */ diff --git a/mm/mprotect.c b/mm/mprotect.c index ec39f73..43a4584 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -568,4 +568,92 @@ static int do_mprotect_pkey(unsigned long start, size_t len, return ret; } +#ifdef CONFIG_SYSFS + +#define PKEYS_ATTR_RO(_name) \ + static struct kobj_attribute _name##_attr = __ATTR_RO(_name) + +static ssize_t total_keys_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + return sprintf(buf, "%u\n", arch_max_pkey()); +} +PKEYS_ATTR_RO(total_keys); + +static ssize_t usable_keys_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + return sprintf(buf, "%u\n", arch_usable_pkeys()); +} +PKEYS_ATTR_RO(usable_keys); + +static ssize_t disable_access_supported_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buf) +{ + if (arch_pkeys_enabled()) { + strcpy(buf, "true\n"); + return sizeof("true\n") - 1; + } + + strcpy(buf, "false\n"); + return sizeof("false\n") - 1; +} +PKEYS_ATTR_RO(disable_access_supported); + +static ssize_t disable_write_supported_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buf) +{ + if (arch_pkeys_enabled()) { + strcpy(buf, "true\n"); + return sizeof("true\n") - 1; + } + + strcpy(buf, "false\n"); + return sizeof("false\n") - 1; +} +PKEYS_ATTR_RO(disable_write_supported); + +static ssize_t disable_execute_supported_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buf) +{ +#ifdef PKEY_DISABLE_EXECUTE + if (arch_supports_pkeys(PKEY_DISABLE_EXECUTE)) { + strcpy(buf, "true\n"); + return sizeof("true\n") - 1; + } +#endif + + strcpy(buf, "false\n"); + return sizeof("false\n") - 1; +} +PKEYS_ATTR_RO(disable_execute_supported); + +static struct attribute *pkeys_attrs[] = { + &total_keys_attr.attr, + &usable_keys_attr.attr, + &disable_access_supported_attr.attr, + &disable_write_supported_attr.attr, + &disable_execute_supported_attr.attr, + NULL, +}; + +static const struct attribute_group pkeys_attr_group = { + .attrs = pkeys_attrs, + .name = "protection_keys", +}; + +static int __init pkeys_sysfs_init(void) +{ + int err; + + err = sysfs_create_group(mm_kobj, &pkeys_attr_group); + + return err; +} +late_initcall(pkeys_sysfs_init); +#endif /* CONFIG_SYSFS */ + #endif /* CONFIG_ARCH_HAS_PKEYS */