[PATCH] xfs: reset buffer pointers before freeing them

[PATCH] xfs: reset buffer pointers before freeing them

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

When we free a vmapped buffer, we need to ensure the vmap address
and length we free is the same as when it was allocated. In various
places in the log code we change the memory the buffer is pointing
to before issuing IO, but we never reset the buffer to point back to
it's original memory (or no memory, if that is the case for the
buffer).

As a result, when we free the buffer it points to memory that is
owned by something else and attempts to unmap and free it. Because
the range does not match any known mapped range, it can trigger
BUG_ON() traps in the vmap code, and potentially corrupt the vmap
area tracking.

Fix this by always resetting these buffers to their original state
before freeing them.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/linux-2.6/xfs_buf.c |   21 +++++++++++++++++++++
 fs/xfs/linux-2.6/xfs_buf.h |    1 +
 fs/xfs/xfs_log.c           |    8 +++++++-
 fs/xfs/xfs_log_recover.c   |   31 ++++++++++++++++++++-----------
 4 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
index d917146..2e8b759 100644
--- a/fs/xfs/linux-2.6/xfs_buf.c
+++ b/fs/xfs/linux-2.6/xfs_buf.c
@@ -711,6 +711,27 @@ xfs_buf_get_empty(
 	return bp;
 }
 
+/*
+ * Return a buffer allocated as an empty buffer and associated to external
+ * memory via xfs_buf_associate_memory() back to it's empty state.
+ */
+void
+xfs_buf_set_empty(
+	struct xfs_buf		*bp,
+	size_t			len)
+{
+	if (bp->b_pages)
+		_xfs_buf_free_pages(bp);
+
+	bp->b_pages = NULL;
+	bp->b_page_count = 0;
+	bp->b_addr = NULL;
+	bp->b_file_offset = 0;
+	bp->b_buffer_length = bp->b_count_desired = len;
+	bp->b_bn = XFS_BUF_DADDR_NULL;
+	bp->b_flags &= ~XBF_MAPPED;
+}
+
 static inline struct page *
 mem_to_page(
 	void			*addr)
diff --git a/fs/xfs/linux-2.6/xfs_buf.h b/fs/xfs/linux-2.6/xfs_buf.h
index a9a1c45..50a7d5f 100644
--- a/fs/xfs/linux-2.6/xfs_buf.h
+++ b/fs/xfs/linux-2.6/xfs_buf.h
@@ -178,6 +178,7 @@ extern xfs_buf_t *xfs_buf_read(xfs_buftarg_t *, xfs_off_t, size_t,
 				xfs_buf_flags_t);
 
 extern xfs_buf_t *xfs_buf_get_empty(size_t, xfs_buftarg_t *);
+extern void xfs_buf_set_empty(struct xfs_buf *bp, size_t len);
 extern xfs_buf_t *xfs_buf_get_uncached(struct xfs_buftarg *, size_t, int);
 extern int xfs_buf_associate_memory(xfs_buf_t *, void *, size_t);
 extern void xfs_buf_hold(xfs_buf_t *);
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b612ce4..3850a91 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1449,6 +1449,13 @@ xlog_dealloc_log(xlog_t *log)
 
 	xlog_cil_destroy(log);
 
+	/*
+	 * always need to ensure that the extra buffer does not point to memory
+	 * owned by another log buffer before we free it.
+	 */
+	xfs_buf_set_empty(log->l_xbuf, log->l_iclog_size);
+	xfs_buf_free(log->l_xbuf);
+
 	iclog = log->l_iclog;
 	for (i=0; i<log->l_iclog_bufs; i++) {
 		xfs_buf_free(iclog->ic_bp);
@@ -1458,7 +1465,6 @@ xlog_dealloc_log(xlog_t *log)
 	}
 	spinlock_destroy(&log->l_icloglock);
 
-	xfs_buf_free(log->l_xbuf);
 	log->l_mp->m_log = NULL;
 	kmem_free(log);
 }	/* xlog_dealloc_log */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 0c4a561..c467e38 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -1229,6 +1229,8 @@ xlog_write_log_records(
 		 */
 		ealign = round_down(end_block, sectbb);
 		if (j == 0 && (start_block + endcount > ealign)) {
+			int error2;
+
 			offset = XFS_BUF_PTR(bp);
 			balign = BBTOB(ealign - start_block);
 			error = XFS_BUF_SET_PTR(bp, offset + balign,
@@ -1237,12 +1239,14 @@ xlog_write_log_records(
 				break;
 
 			error = xlog_bread_noalign(log, ealign, sectbb, bp);
-			if (error)
-				break;
 
-			error = XFS_BUF_SET_PTR(bp, offset, bufblks);
+			/* must reset buffer pointer even on error */
+			error2 = XFS_BUF_SET_PTR(bp, offset, bufblks);
+			if (error2 && !error)
+				error = error2;
 			if (error)
 				break;
+
 		}
 
 		offset = xlog_align(log, start_block, endcount, bp);
@@ -3422,6 +3426,8 @@ xlog_do_recovery_pass(
 				if (error)
 					goto bread_err2;
 			} else {
+				int error2;
+
 				/* This LR is split across physical log end */
 				if (blk_no != log->l_logBBsize) {
 					/* some data before physical log end */
@@ -3456,11 +3462,12 @@ xlog_do_recovery_pass(
 
 				error = xlog_bread_noalign(log, 0,
 							   wrapped_hblks, hbp);
-				if (error)
-					goto bread_err2;
 
-				error = XFS_BUF_SET_PTR(hbp, offset,
+				/* must reset buffer pointer even on error */
+				error2 = XFS_BUF_SET_PTR(hbp, offset,
 							BBTOB(hblks));
+				if (error2 && !error)
+					error = error2;
 				if (error)
 					goto bread_err2;
 			}
@@ -3480,6 +3487,8 @@ xlog_do_recovery_pass(
 				if (error)
 					goto bread_err2;
 			} else {
+				int error2;
+
 				/* This log record is split across the
 				 * physical end of log */
 				offset = XFS_BUF_PTR(dbp);
@@ -3518,12 +3527,12 @@ xlog_do_recovery_pass(
 					goto bread_err2;
 
 				error = xlog_bread_noalign(log, wrapped_hblks,
-						bblks - split_bblks,
-						dbp);
-				if (error)
-					goto bread_err2;
+						bblks - split_bblks, dbp);
 
-				error = XFS_BUF_SET_PTR(dbp, offset, h_size);
+				/* must reset buffer pointer even on error */
+				error2 = XFS_BUF_SET_PTR(dbp, offset, h_size);
+				if (error2 && !error)
+					error = error2;
 				if (error)
 					goto bread_err2;
 			}
-- 
1.7.2.3

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: reset buffer pointers before freeing them

[Christoph Hellwig]

>  			error = xlog_bread_noalign(log, ealign, sectbb, bp);
> -			if (error)
> -				break;
>  
> -			error = XFS_BUF_SET_PTR(bp, offset, bufblks);
> +			/* must reset buffer pointer even on error */
> +			error2 = XFS_BUF_SET_PTR(bp, offset, bufblks);

This seems to be incorrect both in the original and your version.
>From all that I can see XFS_BUF_SET_PTR wants a byte count while bufblks
is a sector count, e.g. this should be:

			error2 = XFS_BUF_SET_PTR(bp, offset, BBTOB(bufblks);

While at I think this mess with the buffer virtual mapping, read into it
and set it back mess needs to go into a single helper instead of beeing
duplicated three times.  By having named arguments with proper types
bugs like the existing one above should also be much more obvious to
spot.

As a follow on patch to that I think we could also get rid of all the
vmap manipultions entirely, and just specify and I/O offset and length
manually.  The only thing required for that is a number of pages
to skip at the beggining of the buffer from the log recovery code to
_xfs_buf_ioapply, either by passing it down procedurally, or by adding
a new filed to struct xfs_buf.  In fact just relaxing the b_offset
semantics to allow offsets larger than a page only in the I/O path would
do it, but I'm not sure that version helps long-term maintenance.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: reset buffer pointers before freeing them

[Dave Chinner]

On Mon, Apr 11, 2011 at 08:09:39PM -0400, Christoph Hellwig wrote:
> >  			error = xlog_bread_noalign(log, ealign, sectbb, bp);
> > -			if (error)
> > -				break;
> >  
> > -			error = XFS_BUF_SET_PTR(bp, offset, bufblks);
> > +			/* must reset buffer pointer even on error */
> > +			error2 = XFS_BUF_SET_PTR(bp, offset, bufblks);
> 
> This seems to be incorrect both in the original and your version.
> From all that I can see XFS_BUF_SET_PTR wants a byte count while bufblks
> is a sector count, e.g. this should be:
> 
> 			error2 = XFS_BUF_SET_PTR(bp, offset, BBTOB(bufblks);

Yes, you are right. I missed that.

> While at I think this mess with the buffer virtual mapping, read into it
> and set it back mess needs to go into a single helper instead of beeing
> duplicated three times.  By having named arguments with proper types
> bugs like the existing one above should also be much more obvious to
> spot.

Ok, I'll try to come up with something sane.

> As a follow on patch to that I think we could also get rid of all the
> vmap manipultions entirely, and just specify and I/O offset and length
> manually.  The only thing required for that is a number of pages
> to skip at the beggining of the buffer from the log recovery code to
> _xfs_buf_ioapply, either by passing it down procedurally, or by adding
> a new filed to struct xfs_buf.  In fact just relaxing the b_offset
> semantics to allow offsets larger than a page only in the I/O path would
> do it, but I'm not sure that version helps long-term maintenance.

Passing the offset+len seems like the cleaner solution to me. I
don't like the idea of overloading existing fields just for the IO
path or adding new fields for one off uses, either. That can
probably wait for another day, though, because we've still got the
iclog buffer hackery to deal with.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: reset buffer pointers before freeing them

[Dave Chinner]

On Tue, Apr 12, 2011 at 09:00:28PM +1000, Dave Chinner wrote:
> On Mon, Apr 11, 2011 at 08:09:39PM -0400, Christoph Hellwig wrote:
> > >  			error = xlog_bread_noalign(log, ealign, sectbb, bp);
> > > -			if (error)
> > > -				break;
> > >  
> > > -			error = XFS_BUF_SET_PTR(bp, offset, bufblks);
> > > +			/* must reset buffer pointer even on error */
> > > +			error2 = XFS_BUF_SET_PTR(bp, offset, bufblks);
> > 
> > This seems to be incorrect both in the original and your version.
> > From all that I can see XFS_BUF_SET_PTR wants a byte count while bufblks
> > is a sector count, e.g. this should be:
> > 
> > 			error2 = XFS_BUF_SET_PTR(bp, offset, BBTOB(bufblks);
> 
> Yes, you are right. I missed that.
> 
> > While at I think this mess with the buffer virtual mapping, read into it
> > and set it back mess needs to go into a single helper instead of beeing
> > duplicated three times.  By having named arguments with proper types
> > bugs like the existing one above should also be much more obvious to
> > spot.
> 
> Ok, I'll try to come up with something sane.

Something like this?


xfs: reset buffer pointers before freeing them

From: Dave Chinner <dchinner@redhat.com>

When we free a vmapped buffer, we need to ensure the vmap address
and length we free is the same as when it was allocated. In various
places in the log code we change the memory the buffer is pointing
to before issuing IO, but we never reset the buffer to point back to
it's original memory (or no memory, if that is the case for the
buffer).

As a result, when we free the buffer it points to memory that is
owned by something else and attempts to unmap and free it. Because
the range does not match any known mapped range, it can trigger
BUG_ON() traps in the vmap code, and potentially corrupt the vmap
area tracking.

Fix this by always resetting these buffers to their original state
before freeing them.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/linux-2.6/xfs_buf.c |   21 ++++++++++++
 fs/xfs/linux-2.6/xfs_buf.h |    1 +
 fs/xfs/xfs_log.c           |    8 ++++-
 fs/xfs/xfs_log_recover.c   |   79 +++++++++++++++++++++++---------------------
 4 files changed, 70 insertions(+), 39 deletions(-)

diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
index 9ef9ed2..ecf0db6 100644
--- a/fs/xfs/linux-2.6/xfs_buf.c
+++ b/fs/xfs/linux-2.6/xfs_buf.c
@@ -709,6 +709,27 @@ xfs_buf_get_empty(
 	return bp;
 }
 
+/*
+ * Return a buffer allocated as an empty buffer and associated to external
+ * memory via xfs_buf_associate_memory() back to it's empty state.
+ */
+void
+xfs_buf_set_empty(
+	struct xfs_buf		*bp,
+	size_t			len)
+{
+	if (bp->b_pages)
+		_xfs_buf_free_pages(bp);
+
+	bp->b_pages = NULL;
+	bp->b_page_count = 0;
+	bp->b_addr = NULL;
+	bp->b_file_offset = 0;
+	bp->b_buffer_length = bp->b_count_desired = len;
+	bp->b_bn = XFS_BUF_DADDR_NULL;
+	bp->b_flags &= ~XBF_MAPPED;
+}
+
 static inline struct page *
 mem_to_page(
 	void			*addr)
diff --git a/fs/xfs/linux-2.6/xfs_buf.h b/fs/xfs/linux-2.6/xfs_buf.h
index a9a1c45..50a7d5f 100644
--- a/fs/xfs/linux-2.6/xfs_buf.h
+++ b/fs/xfs/linux-2.6/xfs_buf.h
@@ -178,6 +178,7 @@ extern xfs_buf_t *xfs_buf_read(xfs_buftarg_t *, xfs_off_t, size_t,
 				xfs_buf_flags_t);
 
 extern xfs_buf_t *xfs_buf_get_empty(size_t, xfs_buftarg_t *);
+extern void xfs_buf_set_empty(struct xfs_buf *bp, size_t len);
 extern xfs_buf_t *xfs_buf_get_uncached(struct xfs_buftarg *, size_t, int);
 extern int xfs_buf_associate_memory(xfs_buf_t *, void *, size_t);
 extern void xfs_buf_hold(xfs_buf_t *);
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index b612ce4..3850a91 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1449,6 +1449,13 @@ xlog_dealloc_log(xlog_t *log)
 
 	xlog_cil_destroy(log);
 
+	/*
+	 * always need to ensure that the extra buffer does not point to memory
+	 * owned by another log buffer before we free it.
+	 */
+	xfs_buf_set_empty(log->l_xbuf, log->l_iclog_size);
+	xfs_buf_free(log->l_xbuf);
+
 	iclog = log->l_iclog;
 	for (i=0; i<log->l_iclog_bufs; i++) {
 		xfs_buf_free(iclog->ic_bp);
@@ -1458,7 +1465,6 @@ xlog_dealloc_log(xlog_t *log)
 	}
 	spinlock_destroy(&log->l_icloglock);
 
-	xfs_buf_free(log->l_xbuf);
 	log->l_mp->m_log = NULL;
 	kmem_free(log);
 }	/* xlog_dealloc_log */
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 5cc464a..fe7fbc8 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -205,6 +205,36 @@ xlog_bread(
 }
 
 /*
+ * Read at an offset into the buffer. Returns with the buffer in it's original
+ * state regardless of the result of the read.
+ */
+STATIC int
+xlog_bread_offset(
+	xlog_t		*log,
+	int		buf_offset,	/* offset into buffer */
+	int		buf_blks,	/* original buffer size */
+	xfs_daddr_t	blk_no,		/* block to read from */
+	int		nbblks,		/* blocks to read */
+	xfs_buf_t	*bp,
+	xfs_caddr_t	offset)
+{
+	xfs_caddr_t	orig_offset = XFS_BUF_PTR(bp);
+	int		error, error2;
+
+	error = XFS_BUF_SET_PTR(bp, offset + BBTOB(buf_offset), BBTOB(nbblks));
+	if (error)
+		return error;
+
+	error = xlog_bread_noalign(log, blk_no, nbblks, bp);
+
+	/* must reset buffer pointer even on error */
+	error2 = XFS_BUF_SET_PTR(bp, orig_offset, BBTOB(buf_blks));
+	if (error)
+		return error;
+	return error2;
+}
+
+/*
  * Write out the buffer at the given block for the given number of blocks.
  * The buffer is kept locked across the write and is returned locked.
  * This can only be used for synchronous log writes.
@@ -1229,20 +1259,12 @@ xlog_write_log_records(
 		 */
 		ealign = round_down(end_block, sectbb);
 		if (j == 0 && (start_block + endcount > ealign)) {
-			offset = XFS_BUF_PTR(bp);
-			balign = BBTOB(ealign - start_block);
-			error = XFS_BUF_SET_PTR(bp, offset + balign,
-						BBTOB(sectbb));
+			balign = ealign - start_block;
+			error = xlog_bread_offset(log, balign, bufblks, ealign,
+						sectbb, bp, XFS_BUF_PTR(bp));
 			if (error)
 				break;
 
-			error = xlog_bread_noalign(log, ealign, sectbb, bp);
-			if (error)
-				break;
-
-			error = XFS_BUF_SET_PTR(bp, offset, bufblks);
-			if (error)
-				break;
 		}
 
 		offset = xlog_align(log, start_block, endcount, bp);
@@ -3322,7 +3344,7 @@ xlog_do_recovery_pass(
 	xfs_caddr_t		offset;
 	xfs_buf_t		*hbp, *dbp;
 	int			error = 0, h_size;
-	int			bblks, split_bblks;
+	int			bblks, split_bblks, wrapped_bblks;
 	int			hblks, split_hblks, wrapped_hblks;
 	struct hlist_head	rhash[XLOG_RHASH_SIZE];
 
@@ -3448,19 +3470,9 @@ xlog_do_recovery_pass(
 				 *   - order is important.
 				 */
 				wrapped_hblks = hblks - split_hblks;
-				error = XFS_BUF_SET_PTR(hbp,
-						offset + BBTOB(split_hblks),
-						BBTOB(hblks - split_hblks));
-				if (error)
-					goto bread_err2;
-
-				error = xlog_bread_noalign(log, 0,
-							   wrapped_hblks, hbp);
-				if (error)
-					goto bread_err2;
-
-				error = XFS_BUF_SET_PTR(hbp, offset,
-							BBTOB(hblks));
+				error = xlog_bread_offset(log, split_hblks,
+							hblks, 0, wrapped_hblks,
+							hbp, offset);
 				if (error)
 					goto bread_err2;
 			}
@@ -3511,19 +3523,10 @@ xlog_do_recovery_pass(
 				 *   _first_, then the log start (LR header end)
 				 *   - order is important.
 				 */
-				error = XFS_BUF_SET_PTR(dbp,
-						offset + BBTOB(split_bblks),
-						BBTOB(bblks - split_bblks));
-				if (error)
-					goto bread_err2;
-
-				error = xlog_bread_noalign(log, wrapped_hblks,
-						bblks - split_bblks,
-						dbp);
-				if (error)
-					goto bread_err2;
-
-				error = XFS_BUF_SET_PTR(dbp, offset, h_size);
+				wrapped_bblks = bblks - split_bblks;
+				error = xlog_bread_offset(log, split_bblks,
+						BTOBB(h_size), 0,
+						wrapped_bblks, dbp, offset);
 				if (error)
 					goto bread_err2;
 			}

Cheers,

Dave
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: reset buffer pointers before freeing them

[Christoph Hellwig]

On Tue, Apr 12, 2011 at 11:45:43PM +1000, Dave Chinner wrote:
> Something like this?

Kinda.  A few comments below.

>  /*
> + * Read at an offset into the buffer. Returns with the buffer in it's original
> + * state regardless of the result of the read.
> + */
> +STATIC int
> +xlog_bread_offset(
> +	xlog_t		*log,
> +	int		buf_offset,	/* offset into buffer */
> +	int		buf_blks,	/* original buffer size */
> +	xfs_daddr_t	blk_no,		/* block to read from */
> +	int		nbblks,		/* blocks to read */
> +	xfs_buf_t	*bp,
> +	xfs_caddr_t	offset)
> +{
> +	xfs_caddr_t	orig_offset = XFS_BUF_PTR(bp);
> +	int		error, error2;
> +
> +	error = XFS_BUF_SET_PTR(bp, offset + BBTOB(buf_offset), BBTOB(nbblks));

Passing in both offset and the buf_offset seems odd as they are only
used together.  So just passing in one byte offset would make more
sense to me.  Of course it's not actually an offset anyore, but virtual
address, but we use that offset naming all around xlog_align/xlog_bread
and callers.

Also I don't think there is a need for the buf_blks argument.  The
argument always comes directly from the xlog_get_bp number blocks
argument, so the

	BBTOB(buf_blks)

can be replaced by simply using a copy of b_buffer_length taken at
the beginning of the function.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfs: reset buffer pointers before freeing them

[Dave Chinner]

On Tue, Apr 12, 2011 at 12:22:37PM -0400, Christoph Hellwig wrote:
> On Tue, Apr 12, 2011 at 11:45:43PM +1000, Dave Chinner wrote:
> > Something like this?
> 
> Kinda.  A few comments below.
> 
> >  /*
> > + * Read at an offset into the buffer. Returns with the buffer in it's original
> > + * state regardless of the result of the read.
> > + */
> > +STATIC int
> > +xlog_bread_offset(
> > +	xlog_t		*log,
> > +	int		buf_offset,	/* offset into buffer */
> > +	int		buf_blks,	/* original buffer size */
> > +	xfs_daddr_t	blk_no,		/* block to read from */
> > +	int		nbblks,		/* blocks to read */
> > +	xfs_buf_t	*bp,
> > +	xfs_caddr_t	offset)
> > +{
> > +	xfs_caddr_t	orig_offset = XFS_BUF_PTR(bp);
> > +	int		error, error2;
> > +
> > +	error = XFS_BUF_SET_PTR(bp, offset + BBTOB(buf_offset), BBTOB(nbblks));
> 
> Passing in both offset and the buf_offset seems odd as they are only
> used together.  So just passing in one byte offset would make more
> sense to me.  Of course it's not actually an offset anyore, but virtual
> address, but we use that offset naming all around xlog_align/xlog_bread
> and callers.

Fair enough. I was struggling with the best way to pass and name
all the parameters. I just posted this before I went to bed to see
if this is what you had in mind....

> Also I don't think there is a need for the buf_blks argument.  The
> argument always comes directly from the xlog_get_bp number blocks
> argument, so the
> 
> 	BBTOB(buf_blks)
> 
> can be replaced by simply using a copy of b_buffer_length taken at
> the beginning of the function.

Very true. My brain surely was not functioning properly ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs