All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] s390: set_fs() removal
@ 2020-09-15 15:43 Heiko Carstens
  2020-09-15 15:43 ` [PATCH 1/4] s390/zcrypt: remove set_fs() invocation in zcrypt device driver Heiko Carstens
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Heiko Carstens @ 2020-09-15 15:43 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

Hi Christoph,

as requested by you s390 specific set_fs() removal patches are
available here:

https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=set_fs

See also this patch series. I'll ask Stephen to get the branch
included in linux-next. The above branch should not be considered
stable.. if something is broken fixes will be merged into the
corresponding commits.
Any review appreciated, of course!

Harald Freudenberger (1):
  s390/zcrypt: remove set_fs() invocation in zcrypt device driver

Heiko Carstens (3):
  s390/dis: get rid of set_fs() usage
  s390/uaccess: implement HAVE_GET_KERNEL_NOFAULT support
  s390/uaccess: remove set_fs() interface

 arch/s390/Kconfig                     |   1 -
 arch/s390/include/asm/futex.h         |  12 +--
 arch/s390/include/asm/mmu_context.h   |   7 +-
 arch/s390/include/asm/processor.h     |   4 +-
 arch/s390/include/asm/uaccess.h       | 129 ++++++++++++++++++++++----
 arch/s390/kernel/dis.c                |  24 +++--
 arch/s390/kernel/entry.S              |  19 +---
 arch/s390/kernel/entry.h              |   1 -
 arch/s390/kernel/process.c            |  15 +--
 arch/s390/lib/uaccess.c               |  76 +++++++--------
 arch/s390/mm/fault.c                  |   9 +-
 arch/s390/mm/pgalloc.c                |  11 +--
 arch/s390/pci/pci_mmio.c              |  12 +--
 drivers/s390/crypto/zcrypt_api.c      |  30 +++---
 drivers/s390/crypto/zcrypt_api.h      |  26 +++++-
 drivers/s390/crypto/zcrypt_ccamisc.c  |  32 ++-----
 drivers/s390/crypto/zcrypt_ep11misc.c |  28 +-----
 drivers/s390/crypto/zcrypt_msgtype6.c |  78 ++++++++--------
 drivers/s390/crypto/zcrypt_msgtype6.h |   4 +-
 19 files changed, 280 insertions(+), 238 deletions(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 1/4] s390/zcrypt: remove set_fs() invocation in zcrypt device driver
  2020-09-15 15:43 [PATCH 0/4] s390: set_fs() removal Heiko Carstens
@ 2020-09-15 15:43 ` Heiko Carstens
  2020-09-15 15:43 ` [PATCH 2/4] s390/dis: get rid of set_fs() usage Heiko Carstens
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Heiko Carstens @ 2020-09-15 15:43 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

From: Harald Freudenberger <freude@linux.ibm.com>

This patch reworks the zcrypt device driver so that the set_fs()
invocation is not needed any more. Instead there is a new flag bool
userspace passed through all the functions which tells if the pointer
arguments are userspace or kernelspace. Together with the two new
inline functions z_copy_from_user() and z_copy_to_user() which either
invoke copy_from_user (userspace is true) or memcpy (userspace is
false) the zcrypt dd and the AP bus now has no requirement for
the set_fs() functionality any more.

Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
Reviewed-by: Ingo Franzki <ifranzki@linux.ibm.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 drivers/s390/crypto/zcrypt_api.c      | 30 +++++------
 drivers/s390/crypto/zcrypt_api.h      | 26 ++++++++-
 drivers/s390/crypto/zcrypt_ccamisc.c  | 32 +++--------
 drivers/s390/crypto/zcrypt_ep11misc.c | 28 ++--------
 drivers/s390/crypto/zcrypt_msgtype6.c | 78 +++++++++++++--------------
 drivers/s390/crypto/zcrypt_msgtype6.h |  4 +-
 6 files changed, 92 insertions(+), 106 deletions(-)

diff --git a/drivers/s390/crypto/zcrypt_api.c b/drivers/s390/crypto/zcrypt_api.c
index 4dbbfd88262c..a711728c3857 100644
--- a/drivers/s390/crypto/zcrypt_api.c
+++ b/drivers/s390/crypto/zcrypt_api.c
@@ -797,7 +797,7 @@ static long zcrypt_rsa_crt(struct ap_perms *perms,
 	return rc;
 }
 
-static long _zcrypt_send_cprb(struct ap_perms *perms,
+static long _zcrypt_send_cprb(bool userspace, struct ap_perms *perms,
 			      struct ica_xcRB *xcRB)
 {
 	struct zcrypt_card *zc, *pref_zc;
@@ -813,7 +813,7 @@ static long _zcrypt_send_cprb(struct ap_perms *perms,
 
 	xcRB->status = 0;
 	ap_init_message(&ap_msg);
-	rc = get_cprb_fc(xcRB, &ap_msg, &func_code, &domain);
+	rc = get_cprb_fc(userspace, xcRB, &ap_msg, &func_code, &domain);
 	if (rc)
 		goto out;
 
@@ -878,7 +878,7 @@ static long _zcrypt_send_cprb(struct ap_perms *perms,
 	if (*domain == AUTOSEL_DOM)
 		*domain = AP_QID_QUEUE(qid);
 
-	rc = pref_zq->ops->send_cprb(pref_zq, xcRB, &ap_msg);
+	rc = pref_zq->ops->send_cprb(userspace, pref_zq, xcRB, &ap_msg);
 
 	spin_lock(&zcrypt_list_lock);
 	zcrypt_drop_queue(pref_zc, pref_zq, mod, weight);
@@ -893,7 +893,7 @@ static long _zcrypt_send_cprb(struct ap_perms *perms,
 
 long zcrypt_send_cprb(struct ica_xcRB *xcRB)
 {
-	return _zcrypt_send_cprb(&ap_perms, xcRB);
+	return _zcrypt_send_cprb(false, &ap_perms, xcRB);
 }
 EXPORT_SYMBOL(zcrypt_send_cprb);
 
@@ -924,7 +924,7 @@ static bool is_desired_ep11_queue(unsigned int dev_qid,
 	return false;
 }
 
-static long _zcrypt_send_ep11_cprb(struct ap_perms *perms,
+static long _zcrypt_send_ep11_cprb(bool userspace, struct ap_perms *perms,
 				   struct ep11_urb *xcrb)
 {
 	struct zcrypt_card *zc, *pref_zc;
@@ -956,7 +956,7 @@ static long _zcrypt_send_ep11_cprb(struct ap_perms *perms,
 		}
 
 		uptr = (struct ep11_target_dev __force __user *) xcrb->targets;
-		if (copy_from_user(targets, uptr,
+		if (z_copy_from_user(userspace, targets, uptr,
 				   target_num * sizeof(*targets))) {
 			func_code = 0;
 			rc = -EFAULT;
@@ -964,7 +964,7 @@ static long _zcrypt_send_ep11_cprb(struct ap_perms *perms,
 		}
 	}
 
-	rc = get_ep11cprb_fc(xcrb, &ap_msg, &func_code);
+	rc = get_ep11cprb_fc(userspace, xcrb, &ap_msg, &func_code);
 	if (rc)
 		goto out_free;
 
@@ -1015,7 +1015,7 @@ static long _zcrypt_send_ep11_cprb(struct ap_perms *perms,
 	}
 
 	qid = pref_zq->queue->qid;
-	rc = pref_zq->ops->send_ep11_cprb(pref_zq, xcrb, &ap_msg);
+	rc = pref_zq->ops->send_ep11_cprb(userspace, pref_zq, xcrb, &ap_msg);
 
 	spin_lock(&zcrypt_list_lock);
 	zcrypt_drop_queue(pref_zc, pref_zq, mod, weight);
@@ -1032,7 +1032,7 @@ static long _zcrypt_send_ep11_cprb(struct ap_perms *perms,
 
 long zcrypt_send_ep11_cprb(struct ep11_urb *xcrb)
 {
-	return _zcrypt_send_ep11_cprb(&ap_perms, xcrb);
+	return _zcrypt_send_ep11_cprb(false, &ap_perms, xcrb);
 }
 EXPORT_SYMBOL(zcrypt_send_ep11_cprb);
 
@@ -1353,12 +1353,12 @@ static int zsecsendcprb_ioctl(struct ap_perms *perms, unsigned long arg)
 	if (copy_from_user(&xcRB, uxcRB, sizeof(xcRB)))
 		return -EFAULT;
 	do {
-		rc = _zcrypt_send_cprb(perms, &xcRB);
+		rc = _zcrypt_send_cprb(true, perms, &xcRB);
 	} while (rc == -EAGAIN);
 	/* on failure: retry once again after a requested rescan */
 	if ((rc == -ENODEV) && (zcrypt_process_rescan()))
 		do {
-			rc = _zcrypt_send_cprb(perms, &xcRB);
+			rc = _zcrypt_send_cprb(true, perms, &xcRB);
 		} while (rc == -EAGAIN);
 	if (rc)
 		ZCRYPT_DBF(DBF_DEBUG, "ioctl ZSENDCPRB rc=%d status=0x%x\n",
@@ -1377,12 +1377,12 @@ static int zsendep11cprb_ioctl(struct ap_perms *perms, unsigned long arg)
 	if (copy_from_user(&xcrb, uxcrb, sizeof(xcrb)))
 		return -EFAULT;
 	do {
-		rc = _zcrypt_send_ep11_cprb(perms, &xcrb);
+		rc = _zcrypt_send_ep11_cprb(true, perms, &xcrb);
 	} while (rc == -EAGAIN);
 	/* on failure: retry once again after a requested rescan */
 	if ((rc == -ENODEV) && (zcrypt_process_rescan()))
 		do {
-			rc = _zcrypt_send_ep11_cprb(perms, &xcrb);
+			rc = _zcrypt_send_ep11_cprb(true, perms, &xcrb);
 		} while (rc == -EAGAIN);
 	if (rc)
 		ZCRYPT_DBF(DBF_DEBUG, "ioctl ZSENDEP11CPRB rc=%d\n", rc);
@@ -1655,12 +1655,12 @@ static long trans_xcRB32(struct ap_perms *perms, struct file *filp,
 	xcRB64.priority_window = xcRB32.priority_window;
 	xcRB64.status = xcRB32.status;
 	do {
-		rc = _zcrypt_send_cprb(perms, &xcRB64);
+		rc = _zcrypt_send_cprb(true, perms, &xcRB64);
 	} while (rc == -EAGAIN);
 	/* on failure: retry once again after a requested rescan */
 	if ((rc == -ENODEV) && (zcrypt_process_rescan()))
 		do {
-			rc = _zcrypt_send_cprb(perms, &xcRB64);
+			rc = _zcrypt_send_cprb(true, perms, &xcRB64);
 		} while (rc == -EAGAIN);
 	xcRB32.reply_control_blk_length = xcRB64.reply_control_blk_length;
 	xcRB32.reply_data_length = xcRB64.reply_data_length;
diff --git a/drivers/s390/crypto/zcrypt_api.h b/drivers/s390/crypto/zcrypt_api.h
index 599e68bf53f7..19ddfc38e029 100644
--- a/drivers/s390/crypto/zcrypt_api.h
+++ b/drivers/s390/crypto/zcrypt_api.h
@@ -59,9 +59,9 @@ struct zcrypt_ops {
 	long (*rsa_modexpo)(struct zcrypt_queue *, struct ica_rsa_modexpo *);
 	long (*rsa_modexpo_crt)(struct zcrypt_queue *,
 				struct ica_rsa_modexpo_crt *);
-	long (*send_cprb)(struct zcrypt_queue *, struct ica_xcRB *,
+	long (*send_cprb)(bool userspace, struct zcrypt_queue *, struct ica_xcRB *,
 			  struct ap_message *);
-	long (*send_ep11_cprb)(struct zcrypt_queue *, struct ep11_urb *,
+	long (*send_ep11_cprb)(bool userspace, struct zcrypt_queue *, struct ep11_urb *,
 			       struct ap_message *);
 	long (*rng)(struct zcrypt_queue *, char *, struct ap_message *);
 	struct list_head list;		/* zcrypt ops list. */
@@ -145,4 +145,26 @@ void zcrypt_device_status_mask_ext(struct zcrypt_device_status_ext *devstatus);
 int zcrypt_device_status_ext(int card, int queue,
 			     struct zcrypt_device_status_ext *devstatus);
 
+static inline unsigned long z_copy_from_user(bool userspace,
+					     void *to,
+					     const void __user *from,
+					     unsigned long n)
+{
+	if (likely(userspace))
+		return copy_from_user(to, from, n);
+	memcpy(to, (void __force *) from, n);
+	return 0;
+}
+
+static inline unsigned long z_copy_to_user(bool userspace,
+					   void __user *to,
+					   const void *from,
+					   unsigned long n)
+{
+	if (likely(userspace))
+		return copy_to_user(to, from, n);
+	memcpy((void __force *) to, from, n);
+	return 0;
+}
+
 #endif /* _ZCRYPT_API_H_ */
diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c b/drivers/s390/crypto/zcrypt_ccamisc.c
index 3f5b61351cde..7d39b43ed3c0 100644
--- a/drivers/s390/crypto/zcrypt_ccamisc.c
+++ b/drivers/s390/crypto/zcrypt_ccamisc.c
@@ -248,24 +248,6 @@ static inline void prep_xcrb(struct ica_xcRB *pxcrb,
 	pxcrb->reply_control_blk_addr = (void __user *) prepcblk;
 }
 
-/*
- * Helper function which calls zcrypt_send_cprb with
- * memory management segment adjusted to kernel space
- * so that the copy_from_user called within this
- * function do in fact copy from kernel space.
- */
-static inline int _zcrypt_send_cprb(struct ica_xcRB *xcrb)
-{
-	int rc;
-	mm_segment_t old_fs = get_fs();
-
-	set_fs(KERNEL_DS);
-	rc = zcrypt_send_cprb(xcrb);
-	set_fs(old_fs);
-
-	return rc;
-}
-
 /*
  * Generate (random) CCA AES DATA secure key.
  */
@@ -359,7 +341,7 @@ int cca_genseckey(u16 cardnr, u16 domain,
 	prep_xcrb(&xcrb, cardnr, preqcblk, prepcblk);
 
 	/* forward xcrb with request CPRB and reply CPRB to zcrypt dd */
-	rc = _zcrypt_send_cprb(&xcrb);
+	rc = zcrypt_send_cprb(&xcrb);
 	if (rc) {
 		DEBUG_ERR("%s zcrypt_send_cprb (cardnr=%d domain=%d) failed, errno %d\n",
 			  __func__, (int) cardnr, (int) domain, rc);
@@ -497,7 +479,7 @@ int cca_clr2seckey(u16 cardnr, u16 domain, u32 keybitsize,
 	prep_xcrb(&xcrb, cardnr, preqcblk, prepcblk);
 
 	/* forward xcrb with request CPRB and reply CPRB to zcrypt dd */
-	rc = _zcrypt_send_cprb(&xcrb);
+	rc = zcrypt_send_cprb(&xcrb);
 	if (rc) {
 		DEBUG_ERR("%s zcrypt_send_cprb (cardnr=%d domain=%d) failed, rc=%d\n",
 			  __func__, (int) cardnr, (int) domain, rc);
@@ -624,7 +606,7 @@ int cca_sec2protkey(u16 cardnr, u16 domain,
 	prep_xcrb(&xcrb, cardnr, preqcblk, prepcblk);
 
 	/* forward xcrb with request CPRB and reply CPRB to zcrypt dd */
-	rc = _zcrypt_send_cprb(&xcrb);
+	rc = zcrypt_send_cprb(&xcrb);
 	if (rc) {
 		DEBUG_ERR("%s zcrypt_send_cprb (cardnr=%d domain=%d) failed, rc=%d\n",
 			  __func__, (int) cardnr, (int) domain, rc);
@@ -850,7 +832,7 @@ int cca_gencipherkey(u16 cardnr, u16 domain, u32 keybitsize, u32 keygenflags,
 	prep_xcrb(&xcrb, cardnr, preqcblk, prepcblk);
 
 	/* forward xcrb with request CPRB and reply CPRB to zcrypt dd */
-	rc = _zcrypt_send_cprb(&xcrb);
+	rc = zcrypt_send_cprb(&xcrb);
 	if (rc) {
 		DEBUG_ERR(
 			"%s zcrypt_send_cprb (cardnr=%d domain=%d) failed, rc=%d\n",
@@ -1018,7 +1000,7 @@ static int _ip_cprb_helper(u16 cardnr, u16 domain,
 	prep_xcrb(&xcrb, cardnr, preqcblk, prepcblk);
 
 	/* forward xcrb with request CPRB and reply CPRB to zcrypt dd */
-	rc = _zcrypt_send_cprb(&xcrb);
+	rc = zcrypt_send_cprb(&xcrb);
 	if (rc) {
 		DEBUG_ERR(
 			"%s zcrypt_send_cprb (cardnr=%d domain=%d) failed, rc=%d\n",
@@ -1235,7 +1217,7 @@ int cca_cipher2protkey(u16 cardnr, u16 domain, const u8 *ckey,
 	prep_xcrb(&xcrb, cardnr, preqcblk, prepcblk);
 
 	/* forward xcrb with request CPRB and reply CPRB to zcrypt dd */
-	rc = _zcrypt_send_cprb(&xcrb);
+	rc = zcrypt_send_cprb(&xcrb);
 	if (rc) {
 		DEBUG_ERR(
 			"%s zcrypt_send_cprb (cardnr=%d domain=%d) failed, rc=%d\n",
@@ -1366,7 +1348,7 @@ int cca_query_crypto_facility(u16 cardnr, u16 domain,
 	prep_xcrb(&xcrb, cardnr, preqcblk, prepcblk);
 
 	/* forward xcrb with request CPRB and reply CPRB to zcrypt dd */
-	rc = _zcrypt_send_cprb(&xcrb);
+	rc = zcrypt_send_cprb(&xcrb);
 	if (rc) {
 		DEBUG_ERR("%s zcrypt_send_cprb (cardnr=%d domain=%d) failed, rc=%d\n",
 			  __func__, (int) cardnr, (int) domain, rc);
diff --git a/drivers/s390/crypto/zcrypt_ep11misc.c b/drivers/s390/crypto/zcrypt_ep11misc.c
index 3c3d403abe92..60b6bec21c32 100644
--- a/drivers/s390/crypto/zcrypt_ep11misc.c
+++ b/drivers/s390/crypto/zcrypt_ep11misc.c
@@ -169,24 +169,6 @@ int ep11_check_aeskeyblob(debug_info_t *dbg, int dbflvl,
 }
 EXPORT_SYMBOL(ep11_check_aeskeyblob);
 
-/*
- * Helper function which calls zcrypt_send_ep11_cprb with
- * memory management segment adjusted to kernel space
- * so that the copy_from_user called within this
- * function do in fact copy from kernel space.
- */
-static inline int _zcrypt_send_ep11_cprb(struct ep11_urb *urb)
-{
-	int rc;
-	mm_segment_t old_fs = get_fs();
-
-	set_fs(KERNEL_DS);
-	rc = zcrypt_send_ep11_cprb(urb);
-	set_fs(old_fs);
-
-	return rc;
-}
-
 /*
  * Allocate and prepare ep11 cprb plus additional payload.
  */
@@ -399,7 +381,7 @@ static int ep11_query_info(u16 cardnr, u16 domain, u32 query_type,
 		 req, sizeof(*req) + sizeof(*req_pl),
 		 rep, sizeof(*rep) + sizeof(*rep_pl) + buflen);
 
-	rc = _zcrypt_send_ep11_cprb(urb);
+	rc = zcrypt_send_ep11_cprb(urb);
 	if (rc) {
 		DEBUG_ERR(
 			"%s zcrypt_send_ep11_cprb(card=%d dom=%d) failed, rc=%d\n",
@@ -637,7 +619,7 @@ int ep11_genaeskey(u16 card, u16 domain, u32 keybitsize, u32 keygenflags,
 		 req, sizeof(*req) + sizeof(*req_pl),
 		 rep, sizeof(*rep) + sizeof(*rep_pl));
 
-	rc = _zcrypt_send_ep11_cprb(urb);
+	rc = zcrypt_send_ep11_cprb(urb);
 	if (rc) {
 		DEBUG_ERR(
 			"%s zcrypt_send_ep11_cprb(card=%d dom=%d) failed, rc=%d\n",
@@ -757,7 +739,7 @@ static int ep11_cryptsingle(u16 card, u16 domain,
 		 req, sizeof(*req) + req_pl_size,
 		 rep, sizeof(*rep) + rep_pl_size);
 
-	rc = _zcrypt_send_ep11_cprb(urb);
+	rc = zcrypt_send_ep11_cprb(urb);
 	if (rc) {
 		DEBUG_ERR(
 			"%s zcrypt_send_ep11_cprb(card=%d dom=%d) failed, rc=%d\n",
@@ -905,7 +887,7 @@ static int ep11_unwrapkey(u16 card, u16 domain,
 		 req, sizeof(*req) + req_pl_size,
 		 rep, sizeof(*rep) + sizeof(*rep_pl));
 
-	rc = _zcrypt_send_ep11_cprb(urb);
+	rc = zcrypt_send_ep11_cprb(urb);
 	if (rc) {
 		DEBUG_ERR(
 			"%s zcrypt_send_ep11_cprb(card=%d dom=%d) failed, rc=%d\n",
@@ -1033,7 +1015,7 @@ static int ep11_wrapkey(u16 card, u16 domain,
 		 req, sizeof(*req) + req_pl_size,
 		 rep, sizeof(*rep) + sizeof(*rep_pl));
 
-	rc = _zcrypt_send_ep11_cprb(urb);
+	rc = zcrypt_send_ep11_cprb(urb);
 	if (rc) {
 		DEBUG_ERR(
 			"%s zcrypt_send_ep11_cprb(card=%d dom=%d) failed, rc=%d\n",
diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c b/drivers/s390/crypto/zcrypt_msgtype6.c
index d77991c74c25..3db901883a5c 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.c
+++ b/drivers/s390/crypto/zcrypt_msgtype6.c
@@ -388,7 +388,7 @@ struct type86_fmt2_msg {
 	struct type86_fmt2_ext fmt2;
 } __packed;
 
-static int XCRB_msg_to_type6CPRB_msgX(struct ap_message *ap_msg,
+static int XCRB_msg_to_type6CPRB_msgX(bool userspace, struct ap_message *ap_msg,
 				      struct ica_xcRB *xcRB,
 				      unsigned int *fcode,
 				      unsigned short **dom)
@@ -465,8 +465,8 @@ static int XCRB_msg_to_type6CPRB_msgX(struct ap_message *ap_msg,
 	msg->hdr.FromCardLen2 = xcRB->reply_data_length;
 
 	/* prepare CPRB */
-	if (copy_from_user(&(msg->cprbx), xcRB->request_control_blk_addr,
-		    xcRB->request_control_blk_length))
+	if (z_copy_from_user(userspace, &(msg->cprbx), xcRB->request_control_blk_addr,
+			     xcRB->request_control_blk_length))
 		return -EFAULT;
 	if (msg->cprbx.cprb_len + sizeof(msg->hdr.function_code) >
 	    xcRB->request_control_blk_length)
@@ -484,16 +484,16 @@ static int XCRB_msg_to_type6CPRB_msgX(struct ap_message *ap_msg,
 
 	/* copy data block */
 	if (xcRB->request_data_length &&
-	    copy_from_user(req_data, xcRB->request_data_address,
-		xcRB->request_data_length))
+	    z_copy_from_user(userspace, req_data, xcRB->request_data_address,
+			     xcRB->request_data_length))
 		return -EFAULT;
 
 	return 0;
 }
 
-static int xcrb_msg_to_type6_ep11cprb_msgx(struct ap_message *ap_msg,
-				       struct ep11_urb *xcRB,
-				       unsigned int *fcode)
+static int xcrb_msg_to_type6_ep11cprb_msgx(bool userspace, struct ap_message *ap_msg,
+					   struct ep11_urb *xcRB,
+					   unsigned int *fcode)
 {
 	unsigned int lfmt;
 	static struct type6_hdr static_type6_ep11_hdr = {
@@ -543,8 +543,8 @@ static int xcrb_msg_to_type6_ep11cprb_msgx(struct ap_message *ap_msg,
 	msg->hdr.FromCardLen1 = xcRB->resp_len;
 
 	/* Import CPRB data from the ioctl input parameter */
-	if (copy_from_user(&(msg->cprbx.cprb_len),
-			   (char __force __user *)xcRB->req, xcRB->req_len)) {
+	if (z_copy_from_user(userspace, &(msg->cprbx.cprb_len),
+			     (char __force __user *)xcRB->req, xcRB->req_len)) {
 		return -EFAULT;
 	}
 
@@ -707,7 +707,7 @@ static int convert_type86_ica(struct zcrypt_queue *zq,
  *
  * Returns 0 on success or -EINVAL, -EFAULT, -EAGAIN in case of an error.
  */
-static int convert_type86_xcrb(struct zcrypt_queue *zq,
+static int convert_type86_xcrb(bool userspace, struct zcrypt_queue *zq,
 			       struct ap_message *reply,
 			       struct ica_xcRB *xcRB)
 {
@@ -715,15 +715,15 @@ static int convert_type86_xcrb(struct zcrypt_queue *zq,
 	char *data = reply->msg;
 
 	/* Copy CPRB to user */
-	if (copy_to_user(xcRB->reply_control_blk_addr,
-		data + msg->fmt2.offset1, msg->fmt2.count1))
+	if (z_copy_to_user(userspace, xcRB->reply_control_blk_addr,
+			   data + msg->fmt2.offset1, msg->fmt2.count1))
 		return -EFAULT;
 	xcRB->reply_control_blk_length = msg->fmt2.count1;
 
 	/* Copy data buffer to user */
 	if (msg->fmt2.count2)
-		if (copy_to_user(xcRB->reply_data_addr,
-			data + msg->fmt2.offset2, msg->fmt2.count2))
+		if (z_copy_to_user(userspace, xcRB->reply_data_addr,
+				   data + msg->fmt2.offset2, msg->fmt2.count2))
 			return -EFAULT;
 	xcRB->reply_data_length = msg->fmt2.count2;
 	return 0;
@@ -738,7 +738,7 @@ static int convert_type86_xcrb(struct zcrypt_queue *zq,
  *
  * Returns 0 on success or -EINVAL, -EFAULT, -EAGAIN in case of an error.
  */
-static int convert_type86_ep11_xcrb(struct zcrypt_queue *zq,
+static int convert_type86_ep11_xcrb(bool userspace, struct zcrypt_queue *zq,
 				    struct ap_message *reply,
 				    struct ep11_urb *xcRB)
 {
@@ -749,8 +749,8 @@ static int convert_type86_ep11_xcrb(struct zcrypt_queue *zq,
 		return -EINVAL;
 
 	/* Copy response CPRB to user */
-	if (copy_to_user((char __force __user *)xcRB->resp,
-			 data + msg->fmt2.offset1, msg->fmt2.count1))
+	if (z_copy_to_user(userspace, (char __force __user *)xcRB->resp,
+			   data + msg->fmt2.offset1, msg->fmt2.count1))
 		return -EFAULT;
 	xcRB->resp_len = msg->fmt2.count1;
 	return 0;
@@ -814,9 +814,9 @@ static int convert_response_ica(struct zcrypt_queue *zq,
 	}
 }
 
-static int convert_response_xcrb(struct zcrypt_queue *zq,
-			    struct ap_message *reply,
-			    struct ica_xcRB *xcRB)
+static int convert_response_xcrb(bool userspace, struct zcrypt_queue *zq,
+				 struct ap_message *reply,
+				 struct ica_xcRB *xcRB)
 {
 	struct type86x_reply *msg = reply->msg;
 
@@ -831,7 +831,7 @@ static int convert_response_xcrb(struct zcrypt_queue *zq,
 			return convert_error(zq, reply);
 		}
 		if (msg->cprbx.cprb_ver_id == 0x02)
-			return convert_type86_xcrb(zq, reply, xcRB);
+			return convert_type86_xcrb(userspace, zq, reply, xcRB);
 		fallthrough;	/* wrong cprb version is an unknown response */
 	default: /* Unknown response type, this should NEVER EVER happen */
 		xcRB->status = 0x0008044DL; /* HDD_InvalidParm */
@@ -848,8 +848,8 @@ static int convert_response_xcrb(struct zcrypt_queue *zq,
 	}
 }
 
-static int convert_response_ep11_xcrb(struct zcrypt_queue *zq,
-	struct ap_message *reply, struct ep11_urb *xcRB)
+static int convert_response_ep11_xcrb(bool userspace, struct zcrypt_queue *zq,
+				      struct ap_message *reply, struct ep11_urb *xcRB)
 {
 	struct type86_ep11_reply *msg = reply->msg;
 
@@ -861,7 +861,7 @@ static int convert_response_ep11_xcrb(struct zcrypt_queue *zq,
 		if (msg->hdr.reply_code)
 			return convert_error(zq, reply);
 		if (msg->cprbx.cprb_ver_id == 0x04)
-			return convert_type86_ep11_xcrb(zq, reply, xcRB);
+			return convert_type86_ep11_xcrb(userspace, zq, reply, xcRB);
 		fallthrough;	/* wrong cprb version is an unknown resp */
 	default: /* Unknown response type, this should NEVER EVER happen */
 		zq->online = 0;
@@ -1095,9 +1095,9 @@ static long zcrypt_msgtype6_modexpo_crt(struct zcrypt_queue *zq,
  * by the caller with ap_init_message(). Also the caller has to
  * make sure ap_release_message() is always called even on failure.
  */
-unsigned int get_cprb_fc(struct ica_xcRB *xcRB,
-				struct ap_message *ap_msg,
-				unsigned int *func_code, unsigned short **dom)
+unsigned int get_cprb_fc(bool userspace, struct ica_xcRB *xcRB,
+			 struct ap_message *ap_msg,
+			 unsigned int *func_code, unsigned short **dom)
 {
 	struct response_type resp_type = {
 		.type = CEXXC_RESPONSE_TYPE_XCRB,
@@ -1112,7 +1112,7 @@ unsigned int get_cprb_fc(struct ica_xcRB *xcRB,
 	ap_msg->private = kmemdup(&resp_type, sizeof(resp_type), GFP_KERNEL);
 	if (!ap_msg->private)
 		return -ENOMEM;
-	return XCRB_msg_to_type6CPRB_msgX(ap_msg, xcRB, func_code, dom);
+	return XCRB_msg_to_type6CPRB_msgX(userspace, ap_msg, xcRB, func_code, dom);
 }
 
 /**
@@ -1122,9 +1122,9 @@ unsigned int get_cprb_fc(struct ica_xcRB *xcRB,
  *	CEXxC device to the request distributor
  * @xcRB: pointer to the send_cprb request buffer
  */
-static long zcrypt_msgtype6_send_cprb(struct zcrypt_queue *zq,
-				    struct ica_xcRB *xcRB,
-				    struct ap_message *ap_msg)
+static long zcrypt_msgtype6_send_cprb(bool userspace, struct zcrypt_queue *zq,
+				      struct ica_xcRB *xcRB,
+				      struct ap_message *ap_msg)
 {
 	int rc;
 	struct response_type *rtype = (struct response_type *)(ap_msg->private);
@@ -1135,7 +1135,7 @@ static long zcrypt_msgtype6_send_cprb(struct zcrypt_queue *zq,
 	if (rc == 0) {
 		rc = ap_msg->rc;
 		if (rc == 0)
-			rc = convert_response_xcrb(zq, ap_msg, xcRB);
+			rc = convert_response_xcrb(userspace, zq, ap_msg, xcRB);
 	} else
 		/* Signal pending. */
 		ap_cancel_message(zq->queue, ap_msg);
@@ -1150,9 +1150,9 @@ static long zcrypt_msgtype6_send_cprb(struct zcrypt_queue *zq,
  * by the caller with ap_init_message(). Also the caller has to
  * make sure ap_release_message() is always called even on failure.
  */
-unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb,
-				    struct ap_message *ap_msg,
-				    unsigned int *func_code)
+unsigned int get_ep11cprb_fc(bool userspace, struct ep11_urb *xcrb,
+			     struct ap_message *ap_msg,
+			     unsigned int *func_code)
 {
 	struct response_type resp_type = {
 		.type = CEXXC_RESPONSE_TYPE_EP11,
@@ -1167,7 +1167,7 @@ unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb,
 	ap_msg->private = kmemdup(&resp_type, sizeof(resp_type), GFP_KERNEL);
 	if (!ap_msg->private)
 		return -ENOMEM;
-	return xcrb_msg_to_type6_ep11cprb_msgx(ap_msg, xcrb, func_code);
+	return xcrb_msg_to_type6_ep11cprb_msgx(userspace, ap_msg, xcrb, func_code);
 }
 
 /**
@@ -1177,7 +1177,7 @@ unsigned int get_ep11cprb_fc(struct ep11_urb *xcrb,
  *	  CEX4P device to the request distributor
  * @xcRB: pointer to the ep11 user request block
  */
-static long zcrypt_msgtype6_send_ep11_cprb(struct zcrypt_queue *zq,
+static long zcrypt_msgtype6_send_ep11_cprb(bool userspace, struct zcrypt_queue *zq,
 					   struct ep11_urb *xcrb,
 					   struct ap_message *ap_msg)
 {
@@ -1237,7 +1237,7 @@ static long zcrypt_msgtype6_send_ep11_cprb(struct zcrypt_queue *zq,
 	if (rc == 0) {
 		rc = ap_msg->rc;
 		if (rc == 0)
-			rc = convert_response_ep11_xcrb(zq, ap_msg, xcrb);
+			rc = convert_response_ep11_xcrb(userspace, zq, ap_msg, xcrb);
 	} else
 		/* Signal pending. */
 		ap_cancel_message(zq->queue, ap_msg);
diff --git a/drivers/s390/crypto/zcrypt_msgtype6.h b/drivers/s390/crypto/zcrypt_msgtype6.h
index 0de280a81dd4..0a0bf074206b 100644
--- a/drivers/s390/crypto/zcrypt_msgtype6.h
+++ b/drivers/s390/crypto/zcrypt_msgtype6.h
@@ -96,9 +96,9 @@ struct type86_fmt2_ext {
 	unsigned int	  offset4;	/* 0x00000000			*/
 } __packed;
 
-unsigned int get_cprb_fc(struct ica_xcRB *, struct ap_message *,
+unsigned int get_cprb_fc(bool userspace, struct ica_xcRB *, struct ap_message *,
 			 unsigned int *, unsigned short **);
-unsigned int get_ep11cprb_fc(struct ep11_urb *, struct ap_message *,
+unsigned int get_ep11cprb_fc(bool userspace, struct ep11_urb *, struct ap_message *,
 			     unsigned int *);
 unsigned int get_rng_fc(struct ap_message *, int *, unsigned int *);
 
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 2/4] s390/dis: get rid of set_fs() usage
  2020-09-15 15:43 [PATCH 0/4] s390: set_fs() removal Heiko Carstens
  2020-09-15 15:43 ` [PATCH 1/4] s390/zcrypt: remove set_fs() invocation in zcrypt device driver Heiko Carstens
@ 2020-09-15 15:43 ` Heiko Carstens
  2020-09-15 15:52   ` Christoph Hellwig
  2020-09-15 15:43 ` [PATCH 3/4] s390/uaccess: implement HAVE_GET_KERNEL_NOFAULT support Heiko Carstens
  2020-09-15 15:43 ` [PATCH 4/4] s390/uaccess: remove set_fs() interface Heiko Carstens
  3 siblings, 1 reply; 14+ messages in thread
From: Heiko Carstens @ 2020-09-15 15:43 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/kernel/dis.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/s390/kernel/dis.c b/arch/s390/kernel/dis.c
index f304802ecf7b..68bf2e9ebe5a 100644
--- a/arch/s390/kernel/dis.c
+++ b/arch/s390/kernel/dis.c
@@ -487,26 +487,30 @@ void show_code(struct pt_regs *regs)
 	char *mode = user_mode(regs) ? "User" : "Krnl";
 	unsigned char code[64];
 	char buffer[128], *ptr;
-	mm_segment_t old_fs;
 	unsigned long addr;
 	int start, end, opsize, hops, i;
 
 	/* Get a snapshot of the 64 bytes surrounding the fault address. */
-	old_fs = get_fs();
-	set_fs(user_mode(regs) ? USER_DS : KERNEL_DS);
 	for (start = 32; start && regs->psw.addr >= 34 - start; start -= 2) {
 		addr = regs->psw.addr - 34 + start;
-		if (__copy_from_user(code + start - 2,
-				     (char __user *) addr, 2))
-			break;
+		if (user_mode(regs)) {
+			if (__copy_from_user(code + start - 2, (char __user *)addr, 2))
+				break;
+		} else {
+			if (copy_from_kernel_nofault(code + start - 2, (char *)addr, 2))
+				break;
+		}
 	}
 	for (end = 32; end < 64; end += 2) {
 		addr = regs->psw.addr + end - 32;
-		if (__copy_from_user(code + end,
-				     (char __user *) addr, 2))
-			break;
+		if (user_mode(regs)) {
+			if (__copy_from_user(code + end, (char __user *)addr, 2))
+				break;
+		} else {
+			if (copy_from_kernel_nofault(code + end, (char *)addr, 2))
+				break;
+		}
 	}
-	set_fs(old_fs);
 	/* Code snapshot useable ? */
 	if ((regs->psw.addr & 1) || start >= end) {
 		printk("%s Code: Bad PSW.\n", mode);
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 3/4] s390/uaccess: implement HAVE_GET_KERNEL_NOFAULT support
  2020-09-15 15:43 [PATCH 0/4] s390: set_fs() removal Heiko Carstens
  2020-09-15 15:43 ` [PATCH 1/4] s390/zcrypt: remove set_fs() invocation in zcrypt device driver Heiko Carstens
  2020-09-15 15:43 ` [PATCH 2/4] s390/dis: get rid of set_fs() usage Heiko Carstens
@ 2020-09-15 15:43 ` Heiko Carstens
  2020-09-15 15:43 ` [PATCH 4/4] s390/uaccess: remove set_fs() interface Heiko Carstens
  3 siblings, 0 replies; 14+ messages in thread
From: Heiko Carstens @ 2020-09-15 15:43 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/include/asm/uaccess.h | 111 ++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index f09444d6aeab..3ef424bca082 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -278,4 +278,115 @@ static inline unsigned long __must_check clear_user(void __user *to, unsigned lo
 int copy_to_user_real(void __user *dest, void *src, unsigned long count);
 void *s390_kernel_write(void *dst, const void *src, size_t size);
 
+#define HAVE_GET_KERNEL_NOFAULT
+
+int __put_kernel_bad(void) __attribute__((noreturn));
+
+#define __put_kernel_asm(val, to, insn)					\
+({									\
+	int __rc;							\
+									\
+	asm volatile(							\
+		"0:   " insn "  %2,%1\n"				\
+		"1:	xr	%0,%0\n"				\
+		"2:\n"							\
+		".pushsection .fixup, \"ax\"\n"				\
+		"3:	lhi	%0,%3\n"				\
+		"	jg	2b\n"					\
+		".popsection\n"						\
+		EX_TABLE(0b,3b) EX_TABLE(1b,3b)				\
+		: "=d" (__rc), "+Q" (*(to))				\
+		: "d" (val), "K" (-EFAULT)				\
+		: "cc");						\
+	__rc;								\
+})
+
+#define __put_kernel_nofault(dst, src, type, err_label)                 \
+do {                                                                    \
+	u64 __x = (u64)(*((type *)(src)));				\
+	int __pk_err;							\
+									\
+	switch (sizeof(type)) {						\
+	case 1:								\
+		__pk_err = __put_kernel_asm(__x, (type *)(dst), "stc"); \
+		break;							\
+	case 2:								\
+		__pk_err = __put_kernel_asm(__x, (type *)(dst), "sth"); \
+		break;							\
+	case 4:								\
+		__pk_err = __put_kernel_asm(__x, (type *)(dst), "st");	\
+		break;							\
+	case 8:								\
+		__pk_err = __put_kernel_asm(__x, (type *)(dst), "stg"); \
+		break;							\
+	default:							\
+		__pk_err = __put_kernel_bad();				\
+		break;							\
+	}								\
+	if (unlikely(__pk_err))						\
+		goto err_label;						\
+} while (0)
+
+int __get_kernel_bad(void) __attribute__((noreturn));
+
+#define __get_kernel_asm(val, from, insn)				\
+({									\
+	int __rc;							\
+									\
+	asm volatile(							\
+		"0:   " insn "  %1,%2\n"				\
+		"1:	xr	%0,%0\n"				\
+		"2:\n"							\
+		".pushsection .fixup, \"ax\"\n"				\
+		"3:	lhi	%0,%3\n"				\
+		"	jg	2b\n"					\
+		".popsection\n"						\
+		EX_TABLE(0b,3b) EX_TABLE(1b,3b)				\
+		: "=d" (__rc), "+d" (val)				\
+		: "Q" (*(from)), "K" (-EFAULT)				\
+		: "cc");						\
+	__rc;								\
+})
+
+#define __get_kernel_nofault(dst, src, type, err_label)                 \
+do {                                                                    \
+	int __gk_err = -EFAULT;						\
+									\
+	switch (sizeof(type)) {						\
+	case 1: {							\
+		u8 __x = 0;						\
+									\
+		__gk_err = __get_kernel_asm(__x, (type *)(src), "ic");	\
+		*((type *)(dst)) = (type)__x;				\
+		break;							\
+	};								\
+	case 2: {							\
+		u16 __x = 0;						\
+									\
+		__gk_err = __get_kernel_asm(__x, (type *)(src), "lh");	\
+		*((type *)(dst)) = (type)__x;				\
+		break;							\
+	};								\
+	case 4: {							\
+		u32 __x = 0;						\
+									\
+		__gk_err = __get_kernel_asm(__x, (type *)(src), "l");	\
+		*((type *)(dst)) = (type)__x;				\
+		break;							\
+	};								\
+	case 8: {							\
+		u64 __x = 0;						\
+									\
+		__gk_err = __get_kernel_asm(__x, (type *)(src), "lg");	\
+		*((type *)(dst)) = (type)__x;				\
+		break;							\
+	};								\
+	default:							\
+		__gk_err = __get_kernel_bad();				\
+		break;							\
+	}								\
+	if (unlikely(__gk_err))						\
+		goto err_label;						\
+} while (0)
+
 #endif /* __S390_UACCESS_H */
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 4/4] s390/uaccess: remove set_fs() interface
  2020-09-15 15:43 [PATCH 0/4] s390: set_fs() removal Heiko Carstens
                   ` (2 preceding siblings ...)
  2020-09-15 15:43 ` [PATCH 3/4] s390/uaccess: implement HAVE_GET_KERNEL_NOFAULT support Heiko Carstens
@ 2020-09-15 15:43 ` Heiko Carstens
  2020-09-15 16:02   ` Christoph Hellwig
  3 siblings, 1 reply; 14+ messages in thread
From: Heiko Carstens @ 2020-09-15 15:43 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

Remove set_fs() interface and therefore the possibility to use uaccess
primitives for kernel address space.  In order to safely access kernel
address space copy_to/from_kernel_nofault() or get_kernel_nofault()
have to be used.

Address spaces still have to switched/changed for machines without the
mvcos instructions and especially for instructions like e.g. compare
and swap (-> futex) which must be executed in kernel address space but
access user address space. For such instructions enable_sacf_uaccess()
and disable_sacf_uaccess() must be used like before.

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/Kconfig                   |  1 -
 arch/s390/include/asm/futex.h       | 12 ++---
 arch/s390/include/asm/mmu_context.h |  7 +--
 arch/s390/include/asm/processor.h   |  4 +-
 arch/s390/include/asm/uaccess.h     | 18 -------
 arch/s390/kernel/entry.S            | 19 ++------
 arch/s390/kernel/entry.h            |  1 -
 arch/s390/kernel/process.c          | 15 +-----
 arch/s390/lib/uaccess.c             | 76 +++++++++++++----------------
 arch/s390/mm/fault.c                |  9 ++--
 arch/s390/mm/pgalloc.c              | 11 ++---
 arch/s390/pci/pci_mmio.c            | 12 ++---
 12 files changed, 63 insertions(+), 122 deletions(-)

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index af72fca9ccd5..b29fcc66ec39 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -185,7 +185,6 @@ config S390
 	select OLD_SIGSUSPEND3
 	select PCI_DOMAINS		if PCI
 	select PCI_MSI			if PCI
-	select SET_FS
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select THREAD_INFO_IN_TASK
diff --git a/arch/s390/include/asm/futex.h b/arch/s390/include/asm/futex.h
index 26f9144562c9..ea225b5a9c80 100644
--- a/arch/s390/include/asm/futex.h
+++ b/arch/s390/include/asm/futex.h
@@ -26,9 +26,9 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
 		u32 __user *uaddr)
 {
 	int oldval = 0, newval, ret;
-	mm_segment_t old_fs;
+	bool old;
 
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	switch (op) {
 	case FUTEX_OP_SET:
 		__futex_atomic_op("lr %2,%5\n",
@@ -53,7 +53,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
 	default:
 		ret = -ENOSYS;
 	}
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 
 	if (!ret)
 		*oval = oldval;
@@ -64,10 +64,10 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
 static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 						u32 oldval, u32 newval)
 {
-	mm_segment_t old_fs;
+	bool old;
 	int ret;
 
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	asm volatile(
 		"   sacf 256\n"
 		"0: cs   %1,%4,0(%5)\n"
@@ -77,7 +77,7 @@ static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 		: "=d" (ret), "+d" (oldval), "=m" (*uaddr)
 		: "0" (-EFAULT), "d" (newval), "a" (uaddr), "m" (*uaddr)
 		: "cc", "memory");
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 	*uval = oldval;
 	return ret;
 }
diff --git a/arch/s390/include/asm/mmu_context.h b/arch/s390/include/asm/mmu_context.h
index c9f3d8a52756..11b8557b1d1d 100644
--- a/arch/s390/include/asm/mmu_context.h
+++ b/arch/s390/include/asm/mmu_context.h
@@ -85,8 +85,9 @@ static inline void clear_user_asce(void)
 	set_cpu_flag(CIF_ASCE_PRIMARY);
 }
 
-mm_segment_t enable_sacf_uaccess(void);
-void disable_sacf_uaccess(mm_segment_t old_fs);
+void setup_address_spaces(void);
+bool enable_sacf_uaccess(void);
+void disable_sacf_uaccess(bool old);
 
 static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
 			     struct task_struct *tsk)
@@ -122,7 +123,7 @@ static inline void finish_arch_post_lock_switch(void)
 		__tlb_flush_mm_lazy(mm);
 		preempt_enable();
 	}
-	set_fs(current->thread.mm_segment);
+	setup_address_spaces();
 }
 
 #define enter_lazy_tlb(mm,tsk)	do { } while (0)
diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
index 962da04234af..5a951527f1bd 100644
--- a/arch/s390/include/asm/processor.h
+++ b/arch/s390/include/asm/processor.h
@@ -102,8 +102,6 @@ extern void __bpon(void);
 
 #define HAVE_ARCH_PICK_MMAP_LAYOUT
 
-typedef unsigned int mm_segment_t;
-
 /*
  * Thread structure
  */
@@ -116,7 +114,7 @@ struct thread_struct {
 	unsigned long hardirq_timer;	/* task cputime in hardirq context */
 	unsigned long softirq_timer;	/* task cputime in softirq context */
 	unsigned long sys_call_table;	/* system call table address */
-	mm_segment_t mm_segment;
+	bool sacf_uaccess;		/* uaccess with sacf enabled */
 	unsigned long gmap_addr;	/* address of last gmap fault. */
 	unsigned int gmap_write_flag;	/* gmap fault write indication */
 	unsigned int gmap_int_code;	/* int code of last gmap fault */
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index 3ef424bca082..f71e93e1ff73 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -18,24 +18,6 @@
 #include <asm/extable.h>
 #include <asm/facility.h>
 
-/*
- * The fs value determines whether argument validity checking should be
- * performed or not.  If get_fs() == USER_DS, checking is performed, with
- * get_fs() == KERNEL_DS, checking is bypassed.
- *
- * For historical reasons, these macros are grossly misnamed.
- */
-
-#define KERNEL_DS	(0)
-#define KERNEL_DS_SACF	(1)
-#define USER_DS		(2)
-#define USER_DS_SACF	(3)
-
-#define get_fs()        (current->thread.mm_segment)
-#define uaccess_kernel() ((get_fs() & 2) == KERNEL_DS)
-
-void set_fs(mm_segment_t fs);
-
 static inline int __range_ok(unsigned long addr, unsigned long size)
 {
 	return 1;
diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
index 23edf196d3dc..434b059bb9fb 100644
--- a/arch/s390/kernel/entry.S
+++ b/arch/s390/kernel/entry.S
@@ -486,19 +486,13 @@ ENTRY(system_call)
 .Lsysc_asce:
 	ni	__LC_CPU_FLAGS+7,255-_CIF_ASCE_SECONDARY
 	lctlg	%c7,%c7,__LC_VDSO_ASCE		# load secondary asce
+#ifndef CONFIG_HAVE_MARCH_Z10_FEATURES
 	TSTMSK	__LC_CPU_FLAGS,_CIF_ASCE_PRIMARY
 	jz	.Lsysc_return
-#ifndef CONFIG_HAVE_MARCH_Z10_FEATURES
-	tm	__LC_STFLE_FAC_LIST+3,0x10	# has MVCOS ?
-	jnz	.Lsysc_set_fs_fixup
 	ni	__LC_CPU_FLAGS+7,255-_CIF_ASCE_PRIMARY
 	lctlg	%c1,%c1,__LC_USER_ASCE		# load primary asce
-	j	.Lsysc_return
-.Lsysc_set_fs_fixup:
 #endif
-	larl	%r14,.Lsysc_return
-	jg	set_fs_fixup
-
+	j	.Lsysc_return
 
 #
 # _TIF_SIGPENDING is set, call do_signal
@@ -877,18 +871,13 @@ ENTRY(io_int_handler)
 .Lio_asce:
 	ni	__LC_CPU_FLAGS+7,255-_CIF_ASCE_SECONDARY
 	lctlg	%c7,%c7,__LC_VDSO_ASCE		# load secondary asce
+#ifndef CONFIG_HAVE_MARCH_Z10_FEATURES
 	TSTMSK	__LC_CPU_FLAGS,_CIF_ASCE_PRIMARY
 	jz	.Lio_return
-#ifndef CONFIG_HAVE_MARCH_Z10_FEATURES
-	tm	__LC_STFLE_FAC_LIST+3,0x10	# has MVCOS ?
-	jnz	.Lio_set_fs_fixup
 	ni	__LC_CPU_FLAGS+7,255-_CIF_ASCE_PRIMARY
 	lctlg	%c1,%c1,__LC_USER_ASCE		# load primary asce
-	j	.Lio_return
-.Lio_set_fs_fixup:
 #endif
-	larl	%r14,.Lio_return
-	jg	set_fs_fixup
+	j	.Lio_return
 
 #
 # CIF_FPU is set, restore floating-point controls and floating-point registers.
diff --git a/arch/s390/kernel/entry.h b/arch/s390/kernel/entry.h
index faca269d5f27..2e9be5657120 100644
--- a/arch/s390/kernel/entry.h
+++ b/arch/s390/kernel/entry.h
@@ -86,7 +86,6 @@ long sys_s390_sthyi(unsigned long function_code, void __user *buffer, u64 __user
 DECLARE_PER_CPU(u64, mt_cycles[8]);
 
 void gs_load_bc_cb(struct pt_regs *regs);
-void set_fs_fixup(void);
 
 unsigned long stack_alloc(void);
 void stack_free(unsigned long stack);
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index ec801d3bbb37..2a87c2dde6fc 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -94,7 +94,7 @@ int copy_thread(unsigned long clone_flags, unsigned long new_stackp,
 	/* Save access registers to new thread structure. */
 	save_access_regs(&p->thread.acrs[0]);
 	/* start new process with ar4 pointing to the correct address space */
-	p->thread.mm_segment = get_fs();
+	p->thread.sacf_uaccess = current->thread.sacf_uaccess;
 	/* Don't copy debug registers */
 	memset(&p->thread.per_user, 0, sizeof(p->thread.per_user));
 	memset(&p->thread.per_event, 0, sizeof(p->thread.per_event));
@@ -208,16 +208,3 @@ unsigned long arch_randomize_brk(struct mm_struct *mm)
 	ret = PAGE_ALIGN(mm->brk + brk_rnd());
 	return (ret > mm->brk) ? ret : mm->brk;
 }
-
-void set_fs_fixup(void)
-{
-	struct pt_regs *regs = current_pt_regs();
-	static bool warned;
-
-	set_fs(USER_DS);
-	if (warned)
-		return;
-	WARN(1, "Unbalanced set_fs - int code: 0x%x\n", regs->int_code);
-	show_registers(regs);
-	warned = true;
-}
diff --git a/arch/s390/lib/uaccess.c b/arch/s390/lib/uaccess.c
index 0267405ab7c6..eb5f2b7360e1 100644
--- a/arch/s390/lib/uaccess.c
+++ b/arch/s390/lib/uaccess.c
@@ -40,61 +40,51 @@ static inline int copy_with_mvcos(void)
 }
 #endif
 
-void set_fs(mm_segment_t fs)
+void setup_address_spaces(void)
 {
-	current->thread.mm_segment = fs;
-	if (fs == USER_DS) {
+	if (likely(!current->thread.sacf_uaccess)) {
 		__ctl_load(S390_lowcore.user_asce, 1, 1);
 		clear_cpu_flag(CIF_ASCE_PRIMARY);
 	} else {
 		__ctl_load(S390_lowcore.kernel_asce, 1, 1);
 		set_cpu_flag(CIF_ASCE_PRIMARY);
-	}
-	if (fs & 1) {
-		if (fs == USER_DS_SACF)
-			__ctl_load(S390_lowcore.user_asce, 7, 7);
-		else
-			__ctl_load(S390_lowcore.kernel_asce, 7, 7);
+		__ctl_load(S390_lowcore.user_asce, 7, 7);
 		set_cpu_flag(CIF_ASCE_SECONDARY);
 	}
 }
-EXPORT_SYMBOL(set_fs);
 
-mm_segment_t enable_sacf_uaccess(void)
+bool enable_sacf_uaccess(void)
 {
-	mm_segment_t old_fs;
 	unsigned long asce, cr;
 	unsigned long flags;
+	bool old;
 
-	old_fs = current->thread.mm_segment;
-	if (old_fs & 1)
-		return old_fs;
+	old = current->thread.sacf_uaccess;
+	if (old)
+		return old;
 	/* protect against a concurrent page table upgrade */
 	local_irq_save(flags);
-	current->thread.mm_segment |= 1;
-	asce = S390_lowcore.kernel_asce;
-	if (likely(old_fs == USER_DS)) {
-		__ctl_store(cr, 1, 1);
-		if (cr != S390_lowcore.kernel_asce) {
-			__ctl_load(S390_lowcore.kernel_asce, 1, 1);
-			set_cpu_flag(CIF_ASCE_PRIMARY);
-		}
-		asce = S390_lowcore.user_asce;
+	current->thread.sacf_uaccess = true;
+	__ctl_store(cr, 1, 1);
+	if (cr != S390_lowcore.kernel_asce) {
+		__ctl_load(S390_lowcore.kernel_asce, 1, 1);
+		set_cpu_flag(CIF_ASCE_PRIMARY);
 	}
+	asce = S390_lowcore.user_asce;
 	__ctl_store(cr, 7, 7);
 	if (cr != asce) {
 		__ctl_load(asce, 7, 7);
 		set_cpu_flag(CIF_ASCE_SECONDARY);
 	}
 	local_irq_restore(flags);
-	return old_fs;
+	return old;
 }
 EXPORT_SYMBOL(enable_sacf_uaccess);
 
-void disable_sacf_uaccess(mm_segment_t old_fs)
+void disable_sacf_uaccess(bool old)
 {
-	current->thread.mm_segment = old_fs;
-	if (old_fs == USER_DS && test_facility(27)) {
+	current->thread.sacf_uaccess = old;
+	if (!old && test_facility(27)) {
 		__ctl_load(S390_lowcore.user_asce, 1, 1);
 		clear_cpu_flag(CIF_ASCE_PRIMARY);
 	}
@@ -135,9 +125,9 @@ static inline unsigned long copy_from_user_mvcp(void *x, const void __user *ptr,
 						unsigned long size)
 {
 	unsigned long tmp1, tmp2;
-	mm_segment_t old_fs;
+	bool old;
 
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	tmp1 = -256UL;
 	asm volatile(
 		"   sacf  0\n"
@@ -164,7 +154,7 @@ static inline unsigned long copy_from_user_mvcp(void *x, const void __user *ptr,
 		EX_TABLE(7b,3b) EX_TABLE(8b,3b) EX_TABLE(9b,6b)
 		: "+a" (size), "+a" (ptr), "+a" (x), "+a" (tmp1), "=a" (tmp2)
 		: : "cc", "memory");
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 	return size;
 }
 
@@ -210,9 +200,9 @@ static inline unsigned long copy_to_user_mvcs(void __user *ptr, const void *x,
 					      unsigned long size)
 {
 	unsigned long tmp1, tmp2;
-	mm_segment_t old_fs;
+	bool old;
 
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	tmp1 = -256UL;
 	asm volatile(
 		"   sacf  0\n"
@@ -239,7 +229,7 @@ static inline unsigned long copy_to_user_mvcs(void __user *ptr, const void *x,
 		EX_TABLE(7b,3b) EX_TABLE(8b,3b) EX_TABLE(9b,6b)
 		: "+a" (size), "+a" (ptr), "+a" (x), "+a" (tmp1), "=a" (tmp2)
 		: : "cc", "memory");
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 	return size;
 }
 
@@ -277,10 +267,10 @@ static inline unsigned long copy_in_user_mvcos(void __user *to, const void __use
 static inline unsigned long copy_in_user_mvc(void __user *to, const void __user *from,
 					     unsigned long size)
 {
-	mm_segment_t old_fs;
 	unsigned long tmp1;
+	bool old;
 
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	asm volatile(
 		"   sacf  256\n"
 		"   aghi  %0,-1\n"
@@ -304,7 +294,7 @@ static inline unsigned long copy_in_user_mvc(void __user *to, const void __user
 		EX_TABLE(1b,6b) EX_TABLE(2b,0b) EX_TABLE(4b,0b)
 		: "+a" (size), "+a" (to), "+a" (from), "=a" (tmp1)
 		: : "cc", "memory");
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 	return size;
 }
 
@@ -346,10 +336,10 @@ static inline unsigned long clear_user_mvcos(void __user *to, unsigned long size
 
 static inline unsigned long clear_user_xc(void __user *to, unsigned long size)
 {
-	mm_segment_t old_fs;
 	unsigned long tmp1, tmp2;
+	bool old;
 
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	asm volatile(
 		"   sacf  256\n"
 		"   aghi  %0,-1\n"
@@ -378,7 +368,7 @@ static inline unsigned long clear_user_xc(void __user *to, unsigned long size)
 		EX_TABLE(1b,6b) EX_TABLE(2b,0b) EX_TABLE(4b,0b)
 		: "+a" (size), "+a" (to), "=a" (tmp1), "=a" (tmp2)
 		: : "cc", "memory");
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 	return size;
 }
 
@@ -414,14 +404,14 @@ static inline unsigned long strnlen_user_srst(const char __user *src,
 
 unsigned long __strnlen_user(const char __user *src, unsigned long size)
 {
-	mm_segment_t old_fs;
 	unsigned long len;
+	bool old;
 
 	if (unlikely(!size))
 		return 0;
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	len = strnlen_user_srst(src, size);
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 	return len;
 }
 EXPORT_SYMBOL(__strnlen_user);
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 4c8c063bce5b..86569da371a1 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -80,17 +80,14 @@ static enum fault_type get_fault_type(struct pt_regs *regs)
 		if (IS_ENABLED(CONFIG_PGSTE) &&
 		    test_pt_regs_flag(regs, PIF_GUEST_FAULT))
 			return GMAP_FAULT;
-		if (current->thread.mm_segment == USER_DS)
+		if (!current->thread.sacf_uaccess)
 			return USER_FAULT;
 		return KERNEL_FAULT;
 	}
 	if (trans_exc_code == 2) {
 		/* secondary space exception */
-		if (current->thread.mm_segment & 1) {
-			if (current->thread.mm_segment == USER_DS_SACF)
-				return USER_FAULT;
-			return KERNEL_FAULT;
-		}
+		if (current->thread.sacf_uaccess)
+			return USER_FAULT;
 		return VDSO_FAULT;
 	}
 	if (trans_exc_code == 1) {
diff --git a/arch/s390/mm/pgalloc.c b/arch/s390/mm/pgalloc.c
index 11d2c8395e2a..0d748645694c 100644
--- a/arch/s390/mm/pgalloc.c
+++ b/arch/s390/mm/pgalloc.c
@@ -73,15 +73,14 @@ static void __crst_table_upgrade(void *arg)
 	/* we must change all active ASCEs to avoid the creation of new TLBs */
 	if (current->active_mm == mm) {
 		S390_lowcore.user_asce = mm->context.asce;
-		if (current->thread.mm_segment == USER_DS) {
-			__ctl_load(S390_lowcore.user_asce, 1, 1);
-			/* Mark user-ASCE present in CR1 */
-			clear_cpu_flag(CIF_ASCE_PRIMARY);
-		}
-		if (current->thread.mm_segment == USER_DS_SACF) {
+		if (current->thread.sacf_uaccess) {
 			__ctl_load(S390_lowcore.user_asce, 7, 7);
 			/* enable_sacf_uaccess does all or nothing */
 			WARN_ON(!test_cpu_flag(CIF_ASCE_SECONDARY));
+		} else {
+			__ctl_load(S390_lowcore.user_asce, 1, 1);
+			/* Mark user-ASCE present in CR1 */
+			clear_cpu_flag(CIF_ASCE_PRIMARY);
 		}
 	}
 	__tlb_flush_local();
diff --git a/arch/s390/pci/pci_mmio.c b/arch/s390/pci/pci_mmio.c
index 401cf670a243..6fff2e5e27df 100644
--- a/arch/s390/pci/pci_mmio.c
+++ b/arch/s390/pci/pci_mmio.c
@@ -93,12 +93,12 @@ static inline int __memcpy_toio_inuser(void __iomem *dst,
 {
 	int size, rc = 0;
 	u8 status = 0;
-	mm_segment_t old_fs;
+	bool old;
 
 	if (!src)
 		return -EINVAL;
 
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	while (n > 0) {
 		size = zpci_get_max_write_size((u64 __force) dst,
 					       (u64 __force) src, n,
@@ -113,7 +113,7 @@ static inline int __memcpy_toio_inuser(void __iomem *dst,
 		dst += size;
 		n -= size;
 	}
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 	if (rc)
 		zpci_err_mmio(rc, status, (__force u64) dst);
 	return rc;
@@ -248,9 +248,9 @@ static inline int __memcpy_fromio_inuser(void __user *dst,
 {
 	int size, rc = 0;
 	u8 status;
-	mm_segment_t old_fs;
+	bool old;
 
-	old_fs = enable_sacf_uaccess();
+	old = enable_sacf_uaccess();
 	while (n > 0) {
 		size = zpci_get_max_write_size((u64 __force) src,
 					       (u64 __force) dst, n,
@@ -262,7 +262,7 @@ static inline int __memcpy_fromio_inuser(void __user *dst,
 		dst += size;
 		n -= size;
 	}
-	disable_sacf_uaccess(old_fs);
+	disable_sacf_uaccess(old);
 	if (rc)
 		zpci_err_mmio(rc, status, (__force u64) dst);
 	return rc;
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] s390/dis: get rid of set_fs() usage
  2020-09-15 15:43 ` [PATCH 2/4] s390/dis: get rid of set_fs() usage Heiko Carstens
@ 2020-09-15 15:52   ` Christoph Hellwig
  2020-09-15 16:55     ` Heiko Carstens
  0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2020-09-15 15:52 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Christoph Hellwig, Vasily Gorbik, Christian Borntraeger,
	Harald Freudenberger, linux-s390

On Tue, Sep 15, 2020 at 05:43:38PM +0200, Heiko Carstens wrote:
> +		if (user_mode(regs)) {
> +			if (__copy_from_user(code + start - 2, (char __user *)addr, 2))
> +				break;
> +		} else {
> +			if (copy_from_kernel_nofault(code + start - 2, (char *)addr, 2))
> +				break;
> +		}

>  	for (end = 32; end < 64; end += 2) {
>  		addr = regs->psw.addr + end - 32;
> +		if (user_mode(regs)) {
> +			if (__copy_from_user(code + end, (char __user *)addr, 2))
> +				break;
> +		} else {
> +			if (copy_from_kernel_nofault(code + end, (char *)addr, 2))
> +				break;
> +		}

Maybe add a little copy_from_regs helper?  That would also get rid
of the awfully long lines here.

Also any good reason for the __copy_from_user instead of the normal
copy_from_user here?

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] s390/uaccess: remove set_fs() interface
  2020-09-15 15:43 ` [PATCH 4/4] s390/uaccess: remove set_fs() interface Heiko Carstens
@ 2020-09-15 16:02   ` Christoph Hellwig
  2020-09-15 16:49     ` Heiko Carstens
  2020-09-15 19:37     ` Heiko Carstens
  0 siblings, 2 replies; 14+ messages in thread
From: Christoph Hellwig @ 2020-09-15 16:02 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Christoph Hellwig, Vasily Gorbik, Christian Borntraeger,
	Harald Freudenberger, linux-s390

On Tue, Sep 15, 2020 at 05:43:40PM +0200, Heiko Carstens wrote:
> Address spaces still have to switched/changed for machines without the
> mvcos instructions and especially for instructions like e.g. compare
> and swap (-> futex) which must be executed in kernel address space but
> access user address space. For such instructions enable_sacf_uaccess()
> and disable_sacf_uaccess() must be used like before.

That logic always confused me and still keeps confusing me,
dumb questions below:

>  	int oldval = 0, newval, ret;
> -	mm_segment_t old_fs;
> +	bool old;
>  
> -	old_fs = enable_sacf_uaccess();
> +	old = enable_sacf_uaccess();
>  	switch (op) {
>  	case FUTEX_OP_SET:
>  		__futex_atomic_op("lr %2,%5\n",
> @@ -53,7 +53,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
>  	default:
>  		ret = -ENOSYS;
>  	}
> -	disable_sacf_uaccess(old_fs);
> +	disable_sacf_uaccess(old);

Do we need to return the old value here?  The way I understand it
this is context switched with the thread, and given that only small
isolated code bases now use it, sacf use can't nest, can it?

> @@ -116,7 +114,7 @@ struct thread_struct {
>  	unsigned long hardirq_timer;	/* task cputime in hardirq context */
>  	unsigned long softirq_timer;	/* task cputime in softirq context */
>  	unsigned long sys_call_table;	/* system call table address */
> -	mm_segment_t mm_segment;
> +	bool sacf_uaccess;		/* uaccess with sacf enabled */

Isn't there a flags field somewhere where this could be folded into?

> -void set_fs_fixup(void)
> -{
> -	struct pt_regs *regs = current_pt_regs();
> -	static bool warned;
> -
> -	set_fs(USER_DS);
> -	if (warned)
> -		return;
> -	WARN(1, "Unbalanced set_fs - int code: 0x%x\n", regs->int_code);
> -	show_registers(regs);
> -	warned = true;

Would a warning about an unbalanced sacf flag still make sense?  Or
just objtool for compile time checks similar to the unsafe uaccess
routines on x86?

> +bool enable_sacf_uaccess(void)

Maybe add a little comment documenting when to use the function

>  }
>  EXPORT_SYMBOL(enable_sacf_uaccess);

Neither enable_sacf_uaccess nor disable_sacf_uaccess appear to be
used in modular code, so these exports can probably be dropped.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] s390/uaccess: remove set_fs() interface
  2020-09-15 16:02   ` Christoph Hellwig
@ 2020-09-15 16:49     ` Heiko Carstens
  2020-09-15 19:37     ` Heiko Carstens
  1 sibling, 0 replies; 14+ messages in thread
From: Heiko Carstens @ 2020-09-15 16:49 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

On Tue, Sep 15, 2020 at 06:02:43PM +0200, Christoph Hellwig wrote:
> >  	int oldval = 0, newval, ret;
> > -	mm_segment_t old_fs;
> > +	bool old;
> >  
> > -	old_fs = enable_sacf_uaccess();
> > +	old = enable_sacf_uaccess();
> >  	switch (op) {
> >  	case FUTEX_OP_SET:
> >  		__futex_atomic_op("lr %2,%5\n",
> > @@ -53,7 +53,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
> >  	default:
> >  		ret = -ENOSYS;
> >  	}
> > -	disable_sacf_uaccess(old_fs);
> > +	disable_sacf_uaccess(old);
> 
> Do we need to return the old value here?  The way I understand it
> this is context switched with the thread, and given that only small
> isolated code bases now use it, sacf use can't nest, can it?

Right, that should not happen. I think I'll change both functions to
void and add a WARN_ON_ONCE() to both of them, so that nested calls
will be catched.

> > @@ -116,7 +114,7 @@ struct thread_struct {
> >  	unsigned long hardirq_timer;	/* task cputime in hardirq context */
> >  	unsigned long softirq_timer;	/* task cputime in softirq context */
> >  	unsigned long sys_call_table;	/* system call table address */
> > -	mm_segment_t mm_segment;
> > +	bool sacf_uaccess;		/* uaccess with sacf enabled */
> 
> Isn't there a flags field somewhere where this could be folded into?

Hmm, a TIF flag will probably do.

> > -void set_fs_fixup(void)
> > -{
> > -	struct pt_regs *regs = current_pt_regs();
> > -	static bool warned;
> > -
> > -	set_fs(USER_DS);
> > -	if (warned)
> > -		return;
> > -	WARN(1, "Unbalanced set_fs - int code: 0x%x\n", regs->int_code);
> > -	show_registers(regs);
> > -	warned = true;
> 
> Would a warning about an unbalanced sacf flag still make sense?  Or
> just objtool for compile time checks similar to the unsafe uaccess
> routines on x86?

Yes, I was not sure to keep it or drop it. But now that you ask for
it, I'll add it back. FWIW, there is no objtool for s390 (yet).

> >  EXPORT_SYMBOL(enable_sacf_uaccess);
> 
> Neither enable_sacf_uaccess nor disable_sacf_uaccess appear to be
> used in modular code, so these exports can probably be dropped.

Ah, I wanted to check, and forgot that.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] s390/dis: get rid of set_fs() usage
  2020-09-15 15:52   ` Christoph Hellwig
@ 2020-09-15 16:55     ` Heiko Carstens
  0 siblings, 0 replies; 14+ messages in thread
From: Heiko Carstens @ 2020-09-15 16:55 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

On Tue, Sep 15, 2020 at 05:52:17PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 15, 2020 at 05:43:38PM +0200, Heiko Carstens wrote:
> > +		if (user_mode(regs)) {
> > +			if (__copy_from_user(code + start - 2, (char __user *)addr, 2))
> > +				break;
> > +		} else {
> > +			if (copy_from_kernel_nofault(code + start - 2, (char *)addr, 2))
> > +				break;
> > +		}
> 
> >  	for (end = 32; end < 64; end += 2) {
> >  		addr = regs->psw.addr + end - 32;
> > +		if (user_mode(regs)) {
> > +			if (__copy_from_user(code + end, (char __user *)addr, 2))
> > +				break;
> > +		} else {
> > +			if (copy_from_kernel_nofault(code + end, (char *)addr, 2))
> > +				break;
> > +		}
> 
> Maybe add a little copy_from_regs helper?  That would also get rid
> of the awfully long lines here.

Yes, I'll check how the result looks like, and will probably change
that.

> Also any good reason for the __copy_from_user instead of the normal
> copy_from_user here?

I don't see any reason. If I remember correctly the only difference is
zero padding, and that wouldn't hurt at all here.
Will check and change.

Thank you for taking a look!

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] s390/uaccess: remove set_fs() interface
  2020-09-15 16:02   ` Christoph Hellwig
  2020-09-15 16:49     ` Heiko Carstens
@ 2020-09-15 19:37     ` Heiko Carstens
  2020-09-16 12:36       ` Heiko Carstens
  1 sibling, 1 reply; 14+ messages in thread
From: Heiko Carstens @ 2020-09-15 19:37 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

On Tue, Sep 15, 2020 at 06:02:43PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 15, 2020 at 05:43:40PM +0200, Heiko Carstens wrote:
> > Address spaces still have to switched/changed for machines without the
> > mvcos instructions and especially for instructions like e.g. compare
> > and swap (-> futex) which must be executed in kernel address space but
> > access user address space. For such instructions enable_sacf_uaccess()
> > and disable_sacf_uaccess() must be used like before.
> 
> That logic always confused me and still keeps confusing me,
> dumb questions below:
> 
> >  	int oldval = 0, newval, ret;
> > -	mm_segment_t old_fs;
> > +	bool old;
> >  
> > -	old_fs = enable_sacf_uaccess();
> > +	old = enable_sacf_uaccess();
> >  	switch (op) {
> >  	case FUTEX_OP_SET:
> >  		__futex_atomic_op("lr %2,%5\n",
> > @@ -53,7 +53,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
> >  	default:
> >  		ret = -ENOSYS;
> >  	}
> > -	disable_sacf_uaccess(old_fs);
> > +	disable_sacf_uaccess(old);
> 
> Do we need to return the old value here?  The way I understand it
> this is context switched with the thread, and given that only small
> isolated code bases now use it, sacf use can't nest, can it?

I just realized that this is broken for uaccess in irq context
(e.g. copy_from_user_nofault()). With set_fs() removal the calls to
force_uaccess_begin()/end() will do nothing, while before
set_fs(USER_DS) actually enforced that control registers on s390 were
setup correctly.
This wouldn't be the case anymore now. If e.g. a code sequence within
enable_sacf_uaccess() would be interrupted, and from within interrupt
context copy_from_user_nofault() would be executed, this would read
from kernel space instead from user space.

Needs fix.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] s390/uaccess: remove set_fs() interface
  2020-09-15 19:37     ` Heiko Carstens
@ 2020-09-16 12:36       ` Heiko Carstens
  2020-11-25  8:38         ` Christoph Hellwig
  0 siblings, 1 reply; 14+ messages in thread
From: Heiko Carstens @ 2020-09-16 12:36 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Vasily Gorbik, Christian Borntraeger, Harald Freudenberger, linux-s390

On Tue, Sep 15, 2020 at 09:37:55PM +0200, Heiko Carstens wrote:
> On Tue, Sep 15, 2020 at 06:02:43PM +0200, Christoph Hellwig wrote:
> > That logic always confused me and still keeps confusing me,
> > dumb questions below:
...
> > > +	bool old;
> > >  
> > > -	old_fs = enable_sacf_uaccess();
> > > +	old = enable_sacf_uaccess();
> > >  	switch (op) {
> > >  	case FUTEX_OP_SET:
> > >  		__futex_atomic_op("lr %2,%5\n",
> > > @@ -53,7 +53,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
> > >  	default:
> > >  		ret = -ENOSYS;
> > >  	}
> > > -	disable_sacf_uaccess(old_fs);
> > > +	disable_sacf_uaccess(old);
> > 
> > Do we need to return the old value here?  The way I understand it
> > this is context switched with the thread, and given that only small
> > isolated code bases now use it, sacf use can't nest, can it?
> 
> I just realized that this is broken for uaccess in irq context
> (e.g. copy_from_user_nofault()). With set_fs() removal the calls to
> force_uaccess_begin()/end() will do nothing, while before
> set_fs(USER_DS) actually enforced that control registers on s390 were
> setup correctly.
> This wouldn't be the case anymore now. If e.g. a code sequence within
> enable_sacf_uaccess() would be interrupted, and from within interrupt
> context copy_from_user_nofault() would be executed, this would read
> from kernel space instead from user space.
> 
> Needs fix.

So, I can think of several ways to fix this (or better: make this
robust). However given that I will be away the next two weeks this is
not going to happen for the upcoming merge window. I really don't want
to rush this, since this has potential for severe subtle bugs... like
we had them already several times with our address space and dynamic
page table upgrade handling in the past (and like I nearly introduced
at least one bug with this patch).

Therefore the first three patches of this series are scheduled for the
upcoming merge window, while the final set_fs() removal should come
one merge later.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] s390/uaccess: remove set_fs() interface
  2020-09-16 12:36       ` Heiko Carstens
@ 2020-11-25  8:38         ` Christoph Hellwig
  2020-11-25  8:51           ` Christian Borntraeger
  0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2020-11-25  8:38 UTC (permalink / raw)
  To: Heiko Carstens
  Cc: Christoph Hellwig, Vasily Gorbik, Christian Borntraeger,
	Harald Freudenberger, linux-s390

On Wed, Sep 16, 2020 at 02:36:03PM +0200, Heiko Carstens wrote:
> So, I can think of several ways to fix this (or better: make this
> robust). However given that I will be away the next two weeks this is
> not going to happen for the upcoming merge window. I really don't want
> to rush this, since this has potential for severe subtle bugs... like
> we had them already several times with our address space and dynamic
> page table upgrade handling in the past (and like I nearly introduced
> at least one bug with this patch).
> 
> Therefore the first three patches of this series are scheduled for the
> upcoming merge window, while the final set_fs() removal should come
> one merge later.

Did you manage to get back to the s390 set_fs removal?

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] s390/uaccess: remove set_fs() interface
  2020-11-25  8:38         ` Christoph Hellwig
@ 2020-11-25  8:51           ` Christian Borntraeger
  2020-11-25 16:36             ` Christoph Hellwig
  0 siblings, 1 reply; 14+ messages in thread
From: Christian Borntraeger @ 2020-11-25  8:51 UTC (permalink / raw)
  To: Christoph Hellwig, Heiko Carstens
  Cc: Vasily Gorbik, Harald Freudenberger, linux-s390



On 25.11.20 09:38, Christoph Hellwig wrote:
> On Wed, Sep 16, 2020 at 02:36:03PM +0200, Heiko Carstens wrote:
>> So, I can think of several ways to fix this (or better: make this
>> robust). However given that I will be away the next two weeks this is
>> not going to happen for the upcoming merge window. I really don't want
>> to rush this, since this has potential for severe subtle bugs... like
>> we had them already several times with our address space and dynamic
>> page table upgrade handling in the past (and like I nearly introduced
>> at least one bug with this patch).
>>
>> Therefore the first three patches of this series are scheduled for the
>> upcoming merge window, while the final set_fs() removal should come
>> one merge later.
> 
> Did you manage to get back to the s390 set_fs removal?
> 

Heiko has queued the following:

https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=features&id=87d5986345219a7e4f204726d9085ea87f3e22d0

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] s390/uaccess: remove set_fs() interface
  2020-11-25  8:51           ` Christian Borntraeger
@ 2020-11-25 16:36             ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2020-11-25 16:36 UTC (permalink / raw)
  To: Christian Borntraeger
  Cc: Christoph Hellwig, Heiko Carstens, Vasily Gorbik,
	Harald Freudenberger, linux-s390

On Wed, Nov 25, 2020 at 09:51:47AM +0100, Christian Borntraeger wrote:
> > Did you manage to get back to the s390 set_fs removal?
> > 
> 
> Heiko has queued the following:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/commit/?h=features&id=87d5986345219a7e4f204726d9085ea87f3e22d0

Ah, nice!

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-11-25 16:36 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 15:43 [PATCH 0/4] s390: set_fs() removal Heiko Carstens
2020-09-15 15:43 ` [PATCH 1/4] s390/zcrypt: remove set_fs() invocation in zcrypt device driver Heiko Carstens
2020-09-15 15:43 ` [PATCH 2/4] s390/dis: get rid of set_fs() usage Heiko Carstens
2020-09-15 15:52   ` Christoph Hellwig
2020-09-15 16:55     ` Heiko Carstens
2020-09-15 15:43 ` [PATCH 3/4] s390/uaccess: implement HAVE_GET_KERNEL_NOFAULT support Heiko Carstens
2020-09-15 15:43 ` [PATCH 4/4] s390/uaccess: remove set_fs() interface Heiko Carstens
2020-09-15 16:02   ` Christoph Hellwig
2020-09-15 16:49     ` Heiko Carstens
2020-09-15 19:37     ` Heiko Carstens
2020-09-16 12:36       ` Heiko Carstens
2020-11-25  8:38         ` Christoph Hellwig
2020-11-25  8:51           ` Christian Borntraeger
2020-11-25 16:36             ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.