Re: [PATCH 2/3] Make sockets proper objhash objects and use checkpoint_obj() on them

[Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

OL> It's perhaps more accurate to s/most sockets/some sockets/. It may
OL> be more likely for a socket to be checkpointed as a peer of
OL> another process, or as the sender of an skb.

Um, how about "most of the time" ?  I definitely think that the
(overwhelmingly) common case is a pair of sockets each attached to a
file descriptor.

OL> Now that you made 'struct sock' a 1st class object, they deserve to
OL> enjoy 1st class treatment :p  That also means proper collect() method
OL> - probably starting with the f_op ...

Okay.

OL> I may be mistaken, but I suspect that the suggested implementation
OL> cannot limit the depth of recursive calls to checkpoint_obj(). For
OL> instance, consider a dgram socket that received data from another
OL> dgram socket, that received data from another dgram, ad infinitum.

At the very least, a single receive socket is limited in how many
skb's may be queued for it, which limits an attacker's ability to
reach the "ad infinitum" case, I'd say.  Do we need something more?

OL> I'm thinking about the two other use cases that I mentioned:
OL> "dangling" (not-referenced by a file) and "pending" (not yet
OL> accepted) sockets.

OL> In both cases (well, at least with "pending"), the 'struct sock'
OL> exist but the 'struct socket' does not exit until after the socket
OL> is attached to a file descriptor. IIRC, the lifespan of 'struct
OL> socket' is coupled to that of the referencing file.

OL> In that case, I guess it make more sense to leave the 'struct
OL> socket' related data within ckpt_hdr_file_socket.

Hmm, not by my reading.  From what I can tell, the accept operation
converts a pending socket into a regular one (for lack of a better
term) by doing a sock_graft().  In fact, it takes the 'struct socket'
of the pending one, and a 'struct socket' attached to the file handle
and grafts the former onto the latter.  So, the pending socket's
'struct socket' *does* exist, unless I'm missing something.

The dangling case isn't quite as clear, so I'll need to investigate
further.  While sock_close() prevents further ->ops calls, I don't see
that it (or ops->release()) actually dissociates the 'struct socket'
from the 'struct sock' until later.

>> +		return ERR_PTR(PTR_ERR(sk));

OL> Nit: I vaguely recall some disapproval of such construct...
OL> How about '(struct file *) sk' ?

Casting it to the wrong type seems less desirable to me.  I was
following the lead of:

  % fgrep -r 'ERR_PTR(PTR_ERR' . | wc -l
  36

and:

  % fgrep -r 'ERR_PTR(PTR_ERR' checkpoint
  checkpoint/namespace.c:		return ERR_PTR(PTR_ERR(h));
  checkpoint/signal.c:		return ERR_PTR(PTR_ERR(h));

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org