C/R support of UNIX sockets
Subsystems (3)
This is my latest UNIX C/R patch set. The helper patches are unchanged from the last versions posted (and acked) on this list. The final UNIX patch includes critical fixes identified by review as well as TODO comments to identify issues that need further work.
I have code to address most of the TODO issues which I will submit to the list after these patches are in the tree.
PATCH 3/5 Add common socket helpers to unify the security hooks
This moves the meat out of the bind(), getsockname(), and getpeername() syscalls into helper functions that performs security_socket_bind() and then the sock->ops->call(). This allows a unification of this behavior between the syscalls and the pending socket restart logic.
--- include/net/sock.h | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ net/socket.c | 29 ++++++----------------------- 2 files changed, 54 insertions(+), 23 deletions(-)
include/net/sock.h +48 -0
@@ -1572,6 +1572,54 @@ extern void sock_enable_timestamp(struct sock *sk, int flag);extern int sock_get_timestamp (struct sock *,struct timeval __user *);extern int sock_get_timestampns (struct sock *,struct timespec __user *); +/* bind() helper shared between any callers needing to perform a bind on +* behalf of userspace (syscall and restart) with the security hooks. +*/ +static inline int sock_bind (struct socket *sock, +struct sockaddr *addr, +int addr_len) +{ +int err; + + err =security_socket_bind (sock, addr, addr_len); +if (err) +return err; +else +return sock->ops->bind (sock, addr, addr_len); +} + +/* getname() helper shared between any callers needing to perform a getname on +* behalf of userspace (syscall and restart) with the security hooks. +*/ +static inline int sock_getname (struct socket *sock, +struct sockaddr *addr, +int *addr_len) +{ +int err; + + err =security_socket_getsockname (sock); +if (err) +return err; +else +return sock->ops->getname (sock, addr, addr_len,0 ); +} + +/* getpeer() helper shared between any callers needing to perform a getpeer on +* behalf of userspace (syscall and restart) with the security hooks. +*/ +static inline int sock_getpeer (struct socket *sock, +struct sockaddr *addr, +int *addr_len) +{ +int err; + + err =security_socket_getpeername (sock); +if (err) +return err; +else +return sock->ops->getname (sock, addr, addr_len,1 ); +} +/* * Enable debug/info messages */
net/socket.c +6 -23
@@ -1414,15 +1414,10 @@ SYSCALL_DEFINE3(bind, int, fd, struct sockaddr __user *, umyaddr, int, addrlen) sock =sockfd_lookup_light (fd, &err, &fput_needed);if (sock) { err =move_addr_to_kernel (umyaddr, addrlen, (struct sockaddr *)&address); -if (err >=0 ) { - err =security_socket_bind (sock, - (struct sockaddr *)&address, - addrlen); -if (!err) - err = sock->ops->bind (sock, - (struct sockaddr *) - &address, addrlen); - } +if (err >=0 ) + err =sock_bind (sock, + (struct sockaddr *)&address, + addrlen);fput_light (sock->file, fput_needed); }return err;
@@ -1610,11 +1605,7 @@ SYSCALL_DEFINE3(getsockname, int, fd, struct sockaddr __user *, usockaddr,if (!sock) goto out; - err =security_socket_getsockname (sock); -if (err) - goto out_put; - - err = sock->ops->getname (sock, (struct sockaddr *)&address, &len,0 ); + err =sock_getname (sock, (struct sockaddr *)&address, &len);if (err) goto out_put; err =move_addr_to_user ((struct sockaddr *)&address, len, usockaddr, usockaddr_len);
@@ -1639,15 +1630,7 @@ SYSCALL_DEFINE3(getpeername, int, fd, struct sockaddr __user *, usockaddr, sock =sockfd_lookup_light (fd, &err, &fput_needed);if (sock !=NULL ) { - err =security_socket_getpeername (sock); -if (err) { -fput_light (sock->file, fput_needed); -return err; - } - - err = - sock->ops->getname (sock, (struct sockaddr *)&address, &len, -1 ); + err =sock_getpeer (sock, (struct sockaddr *)&address, &len);if (!err) err =move_addr_to_user ((struct sockaddr *)&address, len, usockaddr, usockaddr_len);
Signature · 1 line
1.6.2.5
PATCH 5/5 c/r: Add AF_UNIX support (v8)
In reply to Dan SmithThis patch adds basic checkpoint/restart support for AF_UNIX sockets. It has been tested with a single and multiple processes, and with data inflight at the time of checkpoint. It supports socketpair()s, path-based, and abstract sockets.
Changes in v8:
- Fix stale dev_alloc_skb() from before the conversion to skb_clone()
- Fix a couple of broken error paths
- Fix memory leak of kvec.iov_base on successful return from sendmsg()
- Fix condition for deciding when to run sock_cptrst_verify()
- Fix buffer queue copy algorithm to hold the lock during walk(s)
- Log the errno when either getname() or getpeer() fails
- Add comments about ancillary messages in the UNIX queue
- Add TODO comments for credential restore and flags via setsockopt()
- Add TODO comment about strangely-connected dgram sockets and the use of sendmsg(peer)
Changes in v7:
- Fix failure to free iov_base in error path of sock_read_buffer()
- Change sock_read_buffer() to use _ckpt_read_obj_type() to get the header length and then use ckpt_kread() directly to read the payload
- Change sock_read_buffers() to sock_unix_read_buffers() and break out some common functionality to better accommodate the subsequent INET patch
- Generalize sock_unix_getnames() into sock_getnames() so INET can use it
- Change skb_morph() to skb_clone() which uses the more common path and still avoids the copy
- Add check to validate the socket type before creating socket on restore
- Comment the CAP_NET_ADMIN override in sock_read_buffer_hdr
- Strengthen the comment about priming the buffer limits
- Change the objhash functions to deny direct checkpoint of sockets and remove the reference counting function
- Change SOCKET_BUFFERS to SOCKET_QUEUE
- Change this,peer objrefs to signed integers
- Remove names from internal socket structures
- Fix handling of sock_copy_buffers() result
- Use ckpt_fill_fname() instead of d_path() for writing CWD
- Use sock_getname() and sock_getpeer() for proper security hookage
- Return -ENOSYS for unsupported socket families in checkpoint and restart
- Use sock_setsockopt() and sock_getsockopt() where possible to save and restore socket option values
- Check for SOCK_DESTROY flag in the global verify function because none of our supported socket types use it
- Check for SOCK_USE_WRITE_QUEUE in AF_UNIX restore function because that flag should not be used on such a socket
- Check socket state in UNIX restart path to validate the subset of valid values
Changes in v6:
- Moved the socket addresses to the per-type header
- Eliminated the HASCWD flag
- Remove use of ckpt_write_err() in restart paths
- Change the order in which buffers are read so that we can set the socket's limit equal to the size of the image's buffers (if appropriate) and then restore the original values afterwards.
- Use the ckpt_validate_errno() helper
- Add a check to make sure that we didn't restore a (UNIX) socket with any skb's in the send buffer
- Fix up sock_unix_join() to not leave addr uninitialized for socketpair
- Remove inclusion of checkpoint_hdr.h in the socket files
- Make sock_unix_write_cwd() use ckpt_write_string() and use the new ckpt_read_string() for reading the cwd
- Use the restored realcred credentials in sock_unix_join()
- Fix error path of the chdir_and_bind
- Change the algorithm for reloading the socket buffers to use sendmsg() on the socket's peer for better accounting
- For DGRAM sockets, check the backlog value against the system max to avoid letting a restart bypass the overloaded queue length
- Use sock_bind() instead of sock->ops->bind() to gain the security hook
- Change "restart" to "restore" in some of the function names
Changes in v5:
- Change laddr and raddr buffers in socket header to be long enough for INET6 addresses
- Place socket.c and sock.h function definitions inside #ifdef CONFIG_CHECKPOINT
- Add explicit check in sock_unix_makeaddr() to refuse if the checkpoint image specifies an addr length of 0
- Split sock_unix_restart() into a few pieces to facilitate:
- Changed behavior of the unix restore code so that unlinked LISTEN sockets don't do a bind()...unlink()
- Save the base path of a bound socket's path so that we can chdir() to the base before bind() if it is a relative path
- Call bind() for any socket that is not established but has a non-zero-length local address
- Enforce the current sysctl limit on socket buffer size during restart unless the user holds CAP_NET_ADMIN
- Unlink a path-based socket before calling bind()
Changes in v4:
- Changed the signdness of rcvlowat, rcvtimeo, sndtimeo, and backlog to match their struct sock definitions. This should avoid issues with sign extension.
- Add a sock_cptrst_verify() function to be run at restore time to validate several of the values in the checkpoint image against limits, flag masks, etc.
- Write an error string with ctk_write_err() in the obscure cases
- Don't write socket buffers for listen sockets
- Sanity check address lengths before we agree to allocate memory
- Check the result of inserting the peer object in the objhash on restart
- Check return value of sock_cptrst() on restart
- Change logic in remote getname() phase of checkpoint to not fail for closed (et al) sockets
- Eliminate the memory copy while reading socket buffers on restart
Changes in v3:
- Move sock_file_checkpoint() above sock_file_restore()
- Change __sock_file_*() functions to do_sock_file_*()
- Adjust some of the struct cr_hdr_socket alignment
- Improve the sock_copy_buffers() algorithm to avoid locking the source queue for the entire operation
- Fix alignment in the socket header struct(s)
- Move the per-protocol structure (ckpt_hdr_socket_un) out of the common socket header and read/write it separately
- Fix missing call to sock_cptrst() in restore path
- Break out the socket joining into another function
- Fix failure to restore the socket address thus fixing getname()
- Check the state values on restart
- Fix case of state being TCP_CLOSE, which allows dgram sockets to be properly connected (if appropriate) to their peer and maintain the sockaddr for getname() operation
- Fix restoring a listening socket that has been unlink()'d
- Fix checkpointing sockets with an in-flight FD-passing SKB. Fail with EBUSY.
- Fix checkpointing listening sockets with an unaccepted connection. Fail with EBUSY.
- Changed 'un' to 'unix' in function and structure names
Changes in v2:
- Change GFP_KERNEL to GFP_ATOMIC in sock_copy_buffers() (this seems to be rather common in other uses of skb_copy())
- Move the ckpt_hdr_socket structure definition to linux/socket.h
- Fix whitespace issue
- Move sock_file_checkpoint() to net/socket.c for symmetry
--- checkpoint/files.c | 7 + checkpoint/objhash.c | 19 + include/linux/checkpoint_hdr.h | 12 + include/linux/socket.h | 65 +++ include/net/sock.h | 11 + net/Makefile | 2 + net/checkpoint.c | 951 ++++++++++++++++++++++++++++++++++++++++ net/socket.c | 85 ++++ 8 files changed, 1152 insertions(+), 0 deletions(-) create mode 100644 net/checkpoint.c
checkpoint/files.c +7 -0
@@ -21,6 +21,7 @@#include <linux/syscalls.h> #include <linux/checkpoint.h> #include <linux/checkpoint_hdr.h> +#include <net/sock.h> /**************************************************************************
@@ -565,6 +566,12 @@ static struct restore_file_ops restore_file_ops[] = { .file_type = CKPT_FILE_FIFO, .restore = fifo_file_restore, }, +/* socket */ + { + .file_name = "SOCKET", + .file_type = CKPT_FILE_SOCKET, + .restore = sock_file_restore, + }, };static struct file *do_restore_file (struct ckpt_ctx *ctx)
checkpoint/objhash.c +19 -0
@@ -20,6 +20,7 @@#include <linux/user_namespace.h> #include <linux/checkpoint.h> #include <linux/checkpoint_hdr.h> +#include <net/sock.h> struct ckpt_obj ;struct ckpt_obj_ops ;
@@ -234,6 +235,17 @@ static void obj_groupinfo_drop(void *ptr)put_group_info ((struct group_info *) ptr); } +static int obj_sock_grab (void *ptr) +{ +sock_hold ((struct sock *) ptr); +return 0 ; +} + +static void obj_sock_drop (void *ptr) +{ +sock_put ((struct sock *) ptr); +} +static struct ckpt_obj_ops ckpt_obj_ops[] = {/* ignored object */ {
@@ -356,6 +368,13 @@ static struct ckpt_obj_ops ckpt_obj_ops[] = { .checkpoint = checkpoint_groupinfo, .restore = restore_groupinfo, }, +/* sock object */ + { + .obj_name = "SOCKET", + .obj_type = CKPT_OBJ_SOCK, + .ref_drop = obj_sock_drop, + .ref_grab = obj_sock_grab, + }, };
include/linux/checkpoint_hdr.h +12 -0
@@ -88,6 +88,12 @@ enum {CKPT_HDR_SIGHAND =601 , +CKPT_HDR_FD_SOCKET =701 , +CKPT_HDR_SOCKET , +CKPT_HDR_SOCKET_QUEUE , +CKPT_HDR_SOCKET_BUFFER , +CKPT_HDR_SOCKET_UNIX , +CKPT_HDR_TAIL =9001 , CKPT_HDR_ERROR = 9999,
@@ -122,6 +128,7 @@ enum obj_type {CKPT_OBJ_CRED ,CKPT_OBJ_USER ,CKPT_OBJ_GROUPINFO , +CKPT_OBJ_SOCK , CKPT_OBJ_MAX };
@@ -327,6 +334,7 @@ enum file_type {CKPT_FILE_GENERIC ,CKPT_FILE_PIPE ,CKPT_FILE_FIFO , +CKPT_FILE_SOCKET , CKPT_FILE_MAX };
@@ -350,6 +358,10 @@ struct ckpt_hdr_file_pipe {__s32 pipe_objref; } __attribute__((aligned(8))); +struct ckpt_hdr_file_socket { +struct ckpt_hdr_file common; +} __attribute__((aligned (8 ))); +struct ckpt_hdr_utsns {struct ckpt_hdr h;char sysname[__NEW_UTS_LEN +1 ];
include/linux/socket.h +65 -0
76 lines not on this page · raw message
include/net/sock.h +11 -0
@@ -1640,4 +1640,15 @@ extern int sysctl_optmem_max;extern __u32 sysctl_wmem_default;extern __u32 sysctl_rmem_default; +#ifdef CONFIG_CHECKPOINT +/* Checkpoint/Restart Functions */ +struct ckpt_ctx ; +struct ckpt_hdr_socket ; +extern int sock_file_checkpoint (struct ckpt_ctx *,void *); +extern void *sock_file_restore (struct ckpt_ctx *); +extern struct socket *do_sock_file_restore (struct ckpt_ctx *, +struct ckpt_hdr_socket *); +extern int do_sock_file_checkpoint (struct ckpt_ctx *ctx,struct file *file); +#endif +#endif /* _SOCK_H */
net/Makefile +2 -0
@@ -66,3 +66,5 @@ ifeq ($(CONFIG_NET),y) obj-$(CONFIG_SYSCTL) += sysctl_net.o endif obj-$(CONFIG_WIMAX) += wimax/ + +obj-$(CONFIG_CHECKPOINT) += checkpoint.o
net/checkpoint.c new file +951 -0
951 lines not on this page · raw message
net/socket.c +85 -0
103 lines not on this page · raw message
Signature · 1 line
1.6.2.5
Re: [PATCH 5/5] c/r: Add AF_UNIX support (v8)
Quoted 4 lines · Dan Smith
This patch adds basic checkpoint/restart support for
AF_UNIXsockets. It has been tested with a single and multiple processes, and with data inflight at the time of checkpoint. It supports socketpair()s, path-based, and abstract sockets.
Thanks, Dan. Apart from the double-free on error case in sock_copy_buffers(), this looks very good to me. So with that change, please feel free to add:
Oren, pls do consider pulling this into v18.
Dan, do you have any testcases (even just descriptions of how to test, which i'll then try to script) i can add to cr_tests?
thanks,
-serge
Re: [PATCH 5/5] c/r: Add AF_UNIX support (v8)
SH> Thanks, Dan. Apart from the double-free on error case in SH> sock_copy_buffers(), this looks very good to me. So with that SH> change, please feel free to add:
Okay, thanks, I've got that fixed up now.
SH> Dan, do you have any testcases (even just descriptions of how to SH> test, which i'll then try to script) i can add to cr_tests?
Yeah, I've got a big ugly test program that I've rolled automatic tests for most scenarios into. Here's the usage output of the test. I'll get it cleaned up and posted too:
Usage: ./testprog [-t (s|d)] [-wn] TEST
Where:
-t s, -t d: Select STREAM or DGRAM sockets
-w: Wait for a response before restarting
-n: Don't terminate the test on successful restartWhere TEST is one of:
--unix_pair: Pair of sockets using socketpair()
--unix_unlinked: Listen socket with an unlinked path
--unix_pair_path: Pair of sockets using paths
--unix_pair_abstract: Pair of sockets using abstract names
--unix_inflight: Pair of sockets with data in flight --unix_inflight_closed: Pair of sockets, one side closed after preflight
--unix_midaccept: Listen socket with unaccepted peer
--unix_fdpass: Attempt to pass an FD
--unix_passcred: Attempt to pass credentials
--unix_path: Post-restart path-based connect to listen socket
--unix_abstract: Post-restart abstract connect to listen socket
--unix_path_unlink: Like pair_path, but unlink socket before restart
--unix_deadpeer: Close peer socket with data in flight
--unix_testopt: Unix socket setting and testing for a sockopt
--unix_anoninflight: Inflight data with unconnected DGRAM sockets
--inet_connected: Pair of localhost INET4 sockets
--inet6_connected: Pair of localhost INET6 sockets
--nonet: No network, just a simple loop
--sendmail: Try to start up sendmail and checkpoint it
Signature · 3 lines
Dan Smith IBM Linux Technology Center email: [email protected]
Re: [PATCH 5/5] c/r: Add AF_UNIX support (v8)
In reply to Dan SmithDan,
I just noticed that this message wasn't posted last night. So hitting "send" now ... sorry about that.
-----
Before pulling this one, I took a quick look at this patch, and I saw that it still uses skb_morph despite the changelog and my memory...
Can you please verify that this is the latest ?
Also, while trying to pull it, I'd like to ask for three cosmetic changes, if it isn't too much -
1) Move 'struct ckpt_hdr_socket' et-al to checkpoint_hdr.h
2) Move everything that is af_unix specific from net/checkpoint.c to (a new) net/unix/checkpoint.c (prototypes probably in af_unix.h ?)
3) Make sure that af_unix code does not compile (and is not called from net/checkpoint.c) unless CONFIG_UNIX is defined.
I pulled all the other patches already, will add this one once you resend.
-----
Thanks,
Oren.
Quoted 48 lines · Dan Smith
This patch adds basic checkpoint/restart support for
AF_UNIXsockets. It has been tested with a single and multiple processes, and with data inflight at the time of checkpoint. It supports socketpair()s, path-based, and abstract sockets.Changes in v8:
- Fix staledev_alloc_skb()from before the conversion toskb_clone()
- Fix a couple of broken error paths
- Fix memory leak of kvec.iov_base on successful return fromsendmsg()
- Fix condition for deciding when to runsock_cptrst_verify()
- Fix buffer queue copy algorithm to hold the lock during walk(s)
- Log the errno when eithergetname()orgetpeer()fails
- Add comments about ancillary messages in the UNIX queue
- Add TODO comments for credential restore and flags viasetsockopt()
- Add TODO comment about strangely-connected dgram sockets and the use of sendmsg(peer)Changes in v7:
- Fix failure to freeiov_basein error path ofsock_read_buffer()
- Changesock_read_buffer()to use_ckpt_read_obj_type()to get the header length and then useckpt_kread()directly to read the payload
- Changesock_read_buffers()tosock_unix_read_buffers()and break out some common functionality to better accommodate the subsequent INET patch
- Generalizesock_unix_getnames()intosock_getnames()so INET can use it
- Changeskb_morph()toskb_clone()which uses the more common path and still avoids the copy
- Add check to validate the socket type before creating socket on restore
- Comment theCAP_NET_ADMINoverride insock_read_buffer_hdr
- Strengthen the comment about priming the buffer limits
- Change the objhash functions to deny direct checkpoint of sockets and remove the reference counting function
- ChangeSOCKET_BUFFERStoSOCKET_QUEUE
- Change this,peer objrefs to signed integers
- Remove names from internal socket structures
- Fix handling ofsock_copy_buffers()result
- Useckpt_fill_fname()instead ofd_path()for writing CWD
- Usesock_getname()andsock_getpeer()for proper security hookage
- Return-ENOSYSfor unsupported socket families in checkpoint and restart
- Usesock_setsockopt()andsock_getsockopt()where possible to save and restore socket option values
- Check forSOCK_DESTROYflag in the global verify function because none of our supported socket types use it
- Check forSOCK_USE_WRITE_QUEUEinAF_UNIXrestore function because that flag should not be used on such a socket
- Check socket state in UNIX restart path to validate the subset of valid values
[...]
Re: [PATCH 5/5] c/r: Add AF_UNIX support (v8)
OL> Before pulling this one, I took a quick look at this patch, and I OL> saw that it still uses skb_morph despite the changelog and my OL> memory...
That's correct. We've been through several ways of allocating the skb's, so it's definitely confusing. We're back to skb_morph() because I'm pre-allocating them for lock safety when traversing the queues. The only thing that is allocated is the actual skb structure itself; the buffers are still shared like with skb_clone().
OL> 1) Move 'struct ckpt_hdr_socket' et-al to checkpoint_hdr.h
Okay, yeah, I guess we never really resolved the question of where they really belong, but I'll put them back there for now.
OL> 2) Move everything that is af_unix specific from net/checkpoint.c OL> to (a new) net/unix/checkpoint.c (prototypes probably in af_unix.h OL> ?)
Um, I guess I can. That will end up with a bunch of other externally-defined interfaces between the generic and the specific code, but I suppose I don't really have a solid argument against it.
OL> 3) Make sure that af_unix code does not compile (and is not called OL> from net/checkpoint.c) unless CONFIG_UNIX is defined.
Okay.
OL> I pulled all the other patches already, will add this one once OL> you resend.
Alright, well, ignore v9 I guess and hopefully we can settle on a nice even number 10 :)
Signature · 3 lines
Dan Smith IBM Linux Technology Center email: [email protected]
Re: [PATCH 5/5] c/r: Add AF_UNIX support (v8)
Quoted 26 lines · Dan Smith
OL> Before pulling this one, I took a quick look at this patch, and I OL> saw that it still uses
skb_morphdespite the changelog and my OL> memory...That's correct. We've been through several ways of allocating the skb's, so it's definitely confusing. We're back to
skb_morph()because I'm pre-allocating them for lock safety when traversing the queues. The only thing that is allocated is the actual skb structure itself; the buffers are still shared like withskb_clone().OL> 1) Move 'struct
ckpt_hdr_socket' et-al tocheckpoint_hdr.hOkay, yeah, I guess we never really resolved the question of where they really belong, but I'll put them back there for now.
OL> 2) Move everything that is
af_unixspecific fromnet/checkpoint.cOL> to (a new)net/unix/checkpoint.c(prototypes probably inaf_unix.hOL> ?)Um, I guess I can. That will end up with a bunch of other externally-defined interfaces between the generic and the specific code, but I suppose I don't really have a solid argument against it.
OL> 3) Make sure that
af_unixcode does not compile (and is not called OL> fromnet/checkpoint.c) unlessCONFIG_UNIXis defined.
Quoted 8 lines
Okay.
OL> I pulled all the other patches already, will add this one once OL> you resend.
Alright, well, ignore v9 I guess and hopefully we can settle on a nice even number 10 :)
Great !
A message that is not in the archive (sent off-list or not yet received): 1249918379-29414-1-git-send-email-danms-r/[email protected]
PATCH 1/5 Add an errno validation function (v2)
Changes in v2:
- Compare against MAX_ERRNO instead of more stringent but obscure constant
--- include/linux/checkpoint.h | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
include/linux/checkpoint.h +5 -1
@@ -28,7 +28,7 @@#include <linux/user_namespace.h> #include <linux/checkpoint_types.h> #include <linux/checkpoint_hdr.h> - +#include <linux/err.h> /* ckpt_ctx: kflags */ #define CKPT_CTX_CHECKPOINT_BIT 0
@@ -267,6 +267,10 @@ extern void *restore_sighand(struct ckpt_ctx *ctx);memcpy (LIVE ,SAVE , count *sizeof (*SAVE )); \ }while (0 ) +static inline int ckpt_validate_errno(int errno) +{ +return (errno >=0 ) && (errno <MAX_ERRNO ); +}/* debugging flags */ #define CKPT_DBASE 0x1/* anything */
Signature · 1 line
1.6.2.5
A message that is not in the archive (sent off-list or not yet received): 1249918379-29414-2-git-send-email-danms-r/[email protected]
Serge E. Hallyn
Acked-byRe: [PATCH 1/5] Add an errno validation function (v2)
Quoted 1 line · Dan Smith
Signed-off-by Dan Smith <danms-r/[email protected]>
Ok, I do think it should be <= MAX_ERRNO but it's not particularly urgent atm :) So with or without that change:
Quoted 38 lines
Changes in v2:
- Compare againstMAX_ERRNOinstead of more stringent but obscure constant--- include/linux/checkpoint.h | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)include/linux/checkpoint.h +5
-1@@ -28,7 +28,7 @@#include <linux/user_namespace.h> #include <linux/checkpoint_types.h> #include <linux/checkpoint_hdr.h> - +#include <linux/err.h> /* ckpt_ctx: kflags */ #define CKPT_CTX_CHECKPOINT_BIT 0@@ -267,6 +267,10 @@ extern void *restore_sighand(struct ckpt_ctx *ctx);memcpy (LIVE ,SAVE , count *sizeof (*SAVE )); \ }while (0 ) +static inline int ckpt_validate_errno(int errno) +{ +return (errno >=0 ) && (errno <MAX_ERRNO ); +}/* debugging flags */ #define CKPT_DBASE 0x1/* anything */ Signature · 6 lines
1.6.2.5 _______________________________________________ Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org https://lists.linux-foundation.org/mailman/listinfo/containers
A message that is not in the archive (sent off-list or not yet received): 20090810183106.GA24373-r/[email protected]
Re: [PATCH 1/5] Add an errno validation function (v2)
SH> Ok, I do think it should be <= MAX_ERRNO but it's not particularly SH> urgent atm :) So with or without that change:
Yep, me too, I forgot to change that.
SH> Acked-by: Serge Hallyn <serue-r/[email protected]>
So, I'll be glad to re-post this patch (by itself) with the ack and the change, or Oren can just do it when he imports it.
Thanks!
Signature · 3 lines
Dan Smith IBM Linux Technology Center email: danms-r/[email protected]
A message that is not in the archive (sent off-list or not yet received): 87k51b5xi6.fsf-FLMGYpZoEPULwtHQx/[email protected]
Re: [PATCH 1/5] Add an errno validation function (v2)
Fixed at import (left Signed-off-by: as is)
Quoted 12 lines · Dan Smith
SH> Ok, I do think it should be <=
MAX_ERRNObut it's not particularly SH> urgent atm :) So with or without that change:Yep, me too, I forgot to change that.
SH> Acked-by: Serge Hallyn <serue-r/[email protected]>
So, I'll be glad to re-post this patch (by itself) with the ack and the change, or Oren can just do it when he imports it.
Thanks!
PATCH 2/5 Add a ckpt_read_string() function (v3)
In reply to a message not in the archiveAdd a ckpt_read_string() function to allow reading of a variable-length (but length-capped) string from the checkpoint stream.
Changes in v3:
- Return immediately on allocation failure instead of falling through to the inevitable crash
Changes in v2:
- Avoid memcpy() by reading into the allocated buffer directly
--- checkpoint/restart.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/checkpoint.h | 1 + 2 files changed, 37 insertions(+), 0 deletions(-)
checkpoint/restart.c +36 -0
@@ -285,6 +285,42 @@ int ckpt_read_consume(struct ckpt_ctx *ctx, int len, int type)return ret; } +/** +* ckpt_read_string - read a string (variable length) +* @ctx: checkpoint context +* @str: pointer to buffer to store allocated string (caller must kfree()) +* @max: maximum acceptable length +* +* This can be used to read a variable-length string from the checkpoint +* stream. @max limits the size of the resulting buffer. Returns zero on +* success, negative on failure. +*/ +int ckpt_read_string (struct ckpt_ctx *ctx,char **str,int max) +{ +int len; +int ret =0 ; + + *str =NULL ; + + len =_ckpt_read_obj_type (ctx,NULL ,0 ,CKPT_HDR_STRING ); +if (len <0 ) +return len; +else if (len > max) +return -EINVAL ; + + *str =kzalloc (len +1 ,GFP_KERNEL ); +if (!*str) +return -ENOMEM ; + + ret =ckpt_kread (ctx, *str, len); +if (ret <0 ) { +kfree (*str); + *str =NULL ; + } + +return ret; +} +/*********************************************************************** * Restart */
include/linux/checkpoint.h +1 -0
@@ -68,6 +68,7 @@ extern int _ckpt_read_obj_type(struct ckpt_ctx *ctx,extern int _ckpt_read_nbuffer (struct ckpt_ctx *ctx,void *ptr,int len);extern int _ckpt_read_buffer (struct ckpt_ctx *ctx,void *ptr,int len);extern int _ckpt_read_string (struct ckpt_ctx *ctx,void *ptr,int len); +extern int ckpt_read_string (struct ckpt_ctx *ctx,char **str,int max);extern void *ckpt_read_obj_type (struct ckpt_ctx *ctx,int len,int type);extern void *ckpt_read_buf_type (struct ckpt_ctx *ctx,int len,int type);extern int ckpt_read_consume (struct ckpt_ctx *ctx,int len,int type);
Signature · 1 line
1.6.2.5
A message that is not in the archive (sent off-list or not yet received): 1249918379-29414-3-git-send-email-danms-r/[email protected]
Re: [PATCH 2/5] Add a ckpt_read_string() function (v3)
I ended up adding:
int ckpt_read_payload(ctx, void **ptr, int max, int type)
It returns the length of the payload actually read and allocated; caller should free the buffer.
and:
char *ckpt_read_string(ctx, max)
It reads the payload of size up to @max (including terminating null!) into allocated buffer that the caller should free. It returns this buffer.
To play it safe, it forces a '\0' at the end of the buffer, in case a malicious user didn't provide it. This way, the caller can safely assume that it is indeed a null terminated string.
This is slightly different than your prototype, so you need to update the fifth patch.
Oren.
Quoted 76 lines · Dan Smith
Add a
ckpt_read_string()function to allow reading of a variable-length (but length-capped) string from the checkpoint stream.Changes in v3:
- Return immediately on allocation failure instead of falling through to the inevitable crashChanges in v2:
- Avoidmemcpy()by reading into the allocated buffer directlySigned-off-by Dan Smith <danms-r/[email protected]>--- checkpoint/restart.c | 36 ++++++++++++++++++++++++++++++++++++ include/linux/checkpoint.h | 1 + 2 files changed, 37 insertions(+), 0 deletions(-)checkpoint/restart.c +36
-0@@ -285,6 +285,42 @@ int ckpt_read_consume(struct ckpt_ctx *ctx, int len, int type)return ret; } +/** +* ckpt_read_string - read a string (variable length) +* @ctx: checkpoint context +* @str: pointer to buffer to store allocated string (caller must kfree()) +* @max: maximum acceptable length +* +* This can be used to read a variable-length string from the checkpoint +* stream. @max limits the size of the resulting buffer. Returns zero on +* success, negative on failure. +*/ +int ckpt_read_string (struct ckpt_ctx *ctx,char **str,int max) +{ +int len; +int ret =0 ; + + *str =NULL ; + + len =_ckpt_read_obj_type (ctx,NULL ,0 ,CKPT_HDR_STRING ); +if (len <0 ) +return len; +else if (len > max) +return -EINVAL ; + + *str =kzalloc (len +1 ,GFP_KERNEL ); +if (!*str) +return -ENOMEM ; + + ret =ckpt_kread (ctx, *str, len); +if (ret <0 ) { +kfree (*str); + *str =NULL ; + } + +return ret; +} +/*********************************************************************** * Restart */ include/linux/checkpoint.h +1
-0@@ -68,6 +68,7 @@ extern int _ckpt_read_obj_type(struct ckpt_ctx *ctx,extern int _ckpt_read_nbuffer (struct ckpt_ctx *ctx,void *ptr,int len);extern int _ckpt_read_buffer (struct ckpt_ctx *ctx,void *ptr,int len);extern int _ckpt_read_string (struct ckpt_ctx *ctx,void *ptr,int len); +extern int ckpt_read_string (struct ckpt_ctx *ctx,char **str,int max);extern void *ckpt_read_obj_type (struct ckpt_ctx *ctx,int len,int type);extern void *ckpt_read_buf_type (struct ckpt_ctx *ctx,int len,int type);extern int ckpt_read_consume (struct ckpt_ctx *ctx,int len,int type);
PATCH 4/5 Export fill_fname() as ckpt_fill_fname()
In reply to a message not in the archive--- checkpoint/files.c | 8 ++++---- include/linux/checkpoint.h | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-)
checkpoint/files.c +4 -4
@@ -28,14 +28,14 @@*/ /** -* fill_fname - return pathname of a given file +* ckpt_fill_fname - return pathname of a given file * @path: path name * @root: relative root * @buf: buffer for pathname * @len: buffer length (in) and pathname length (out) */ -static char *fill_fname (struct path *path,struct path *root, -char *buf,int *len) +char *ckpt_fill_fname (struct path *path,struct path *root, +char *buf,int *len) {struct path tmp = *root;char *fname;
@@ -79,7 +79,7 @@ int checkpoint_fname(struct ckpt_ctx *ctx, struct path *path, struct path *root)if (!buf)return -ENOMEM ; - fname =fill_fname (path, root, buf, &flen); + fname =ckpt_fill_fname (path, root, buf, &flen);if (!IS_ERR (fname)) ret =ckpt_write_obj_type (ctx, fname, flen,CKPT_HDR_FILE_NAME );
include/linux/checkpoint.h +2 -0
@@ -72,6 +72,8 @@ extern int ckpt_read_string(struct ckpt_ctx *ctx, char **str, int max);extern void *ckpt_read_obj_type (struct ckpt_ctx *ctx,int len,int type);extern void *ckpt_read_buf_type (struct ckpt_ctx *ctx,int len,int type);extern int ckpt_read_consume (struct ckpt_ctx *ctx,int len,int type); +extern char *ckpt_fill_fname (struct path *path,struct path *root, +char *buf,int *len);/* ckpt kflags */ #define ckpt_set_ctx_kflag (__ctx, __kflag) \
Signature · 1 line
1.6.2.5
Pulled the first four into ckpt-v17-dev.
See my reply to the fifth.
Oren.
Quoted 8 lines · Dan Smith
This is my latest UNIX C/R patch set. The helper patches are unchanged from the last versions posted (and acked) on this list. The final UNIX patch includes critical fixes identified by review as well as TODO comments to identify issues that need further work.
I have code to address most of the TODO issues which I will submit to the list after these patches are in the tree.